Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

[Guangya Liu]

> On July 5, 2016, 4:47 p.m., Gilbert Song wrote:
> > src/tests/containerizer/appc_spec_tests.cpp, line 80
> > 
> >
> > We should firstly check `has_workingDirectory()` right? Otherwise, it 
> > may segfault if not existed.

+1 to check `has_workingDirectory()` here but as we already defined the 
manifest above and it does include a `workingDirectory` there, so will not 
cause issue here.


- Guangya


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


On June 30, 2016, 6 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated June 30, 2016, 6 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49689: Added Appc runtime isolator tests.

[Guangya Liu]

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



Just some early comments.


src/tests/containerizer/appc_archive.hpp (lines 25 - 26)


adjut the order here



src/tests/containerizer/appc_archive.hpp (lines 38 - 42)


This isn't allowed in a header.  (We don't want the namespace(s) to be 
polluted with unnecessary aliases.)



src/tests/containerizer/appc_archive.hpp (line 48)


s/a appc/an Appc



src/tests/containerizer/appc_archive.hpp (line 53)


s/a appc/an Appc



src/tests/containerizer/appc_archive.hpp (lines 54 - 55)


I think you can remove this as this does not match the function here.



src/tests/containerizer/appc_archive.hpp (line 56)


What about define this as `static Future create(` just like what 
we did for docker 
https://github.com/apache/mesos/blob/master/src/tests/containerizer/docker_archive.hpp#L51



src/tests/containerizer/appc_archive.hpp (lines 122 - 126)


Once the tar image succeed, we should delete the image directory under 
`imagePath`, otherwise, there will be some garbage data after test.



src/tests/containerizer/appc_archive.hpp (line 128)


You will not need this if return a `Future`



src/tests/containerizer/appc_runtime_isolator_tests.cpp (lines 20 - 21)


adjust the order here



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 69)


adjust the indent here



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 70)


new line above



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 91)


I think you do not need the master flag here, ditto for other test cases.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (lines 126 - 127)


new line here



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 214)


new line above



src/tests/containerizer/appc_runtime_isolator_tests.cpp (lines 252 - 253)


This should be an error case according to the isolator logic table.



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 301)


new line above



src/tests/containerizer/appc_runtime_isolator_tests.cpp (line 386)


1) This case is testing `sh=1` but here `shell` is false.
2) new line above


- Guangya Liu


On July 6, 2016, 6:34 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49689/
> ---
> 
> (Updated July 6, 2016, 6:34 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP. Added Appc runtime isolator tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 28dd15166937ed672f81be5a598df149b8ed4302 
>   src/tests/containerizer/appc_archive.hpp PRE-CREATION 
>   src/tests/containerizer/appc_runtime_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49689/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49445: Updated FilesProcess to support List_Files Call in Operator API v1.

[Abhishek Dasgupta]

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

(Updated July 7, 2016, 5:25 a.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Updated FilesProcess to support List_Files Call in Operator API v1.


Diffs (updated)
-

  src/files/files.hpp 06a91a53b62ba930afaf116b9b6e90cab9ecd515 
  src/files/files.cpp 0368f67fc184dbd29945fcb9216439f0a5b713bd 
  src/tests/files_tests.cpp a30e452536f395c6e6286e75cb267fcc213bac44 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49571: Added a benchmark test for allocations.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49571, 45967, 45966, 45964, 45963, 45962, 45961, 45960, 
48616, 45959, 45958]

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

Error:
2016-07-07 04:01:07 URL:https://reviews.apache.org/r/45961/diff/raw/ 
[34967/34967] -> "45961.patch" [1]
error: patch failed: src/master/allocator/sorter/drf/sorter.cpp:296
error: src/master/allocator/sorter/drf/sorter.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/14192/console

- Mesos ReviewBot


On July 7, 2016, 2:20 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated July 7, 2016, 2:20 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This parameterized test has the following configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed. Following results show that the support of shared resources 
> has a small impact on runtime performance in allocations between HEAD and 
> this patch. Also, there is no visible impact in performance when shared 
> resources are added in the tests.
> 
> Based on HEAD, with all regular resources (no shared resources in HEAD 
> supported):
> round 0 allocate took 3.074583secs to make 200 offers
> round 50 allocate took 3.124076secs to make 200 offers
> round 100 allocate took 3.136869secs to make 200 offers
> round 150 allocate took 3.081407secs to make 200 offers
> round 199 allocate took 3.087184secs to make 200 offers
> 
> With the patch (and no shared resources):
> round 0 allocate took 3.297495secs to make 200 offers
> round 50 allocate took 3.318641secs to make 200 offers
> round 100 allocate took 3.312251secs to make 200 offers
> round 150 allocate took 3.318052secs to make 200 offers
> round 199 allocate took 3.3061secs to make 200 offers
> 
> With the patch (and shared resources on all agents):
> round 0 allocate took 3.370602secs to make 200 offers
> round 50 allocate took 3.380281secs to make 200 offers
> round 100 allocate took 3.369976secs to make 200 offers
> round 150 allocate took 3.372334secs to make 200 offers
> round 199 allocate took 3.372049secs to make 200 offers
> 
> With the patch (and shared resources on alternate agents):
> round 0 allocate took 3.360305secs to make 200 offers
> round 50 allocate took 3.370903secs to make 200 offers
> round 100 allocate took 3.369496secs to make 200 offers
> round 150 allocate took 3.363246secs to make 200 offers
> round 199 allocate took 3.485207secs to make 200 offers
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49736: Added PATCH_CMD in 3rdparty/CMakeLists.txt for ELFIO.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49736]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 6, 2016, 11:58 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49736/
> ---
> 
> (Updated July 6, 2016, 11:58 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added PATCH_CMD in 3rdparty/CMakeLists.txt for ELFIO.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt a7ae76a40fcf5c6d6c9dc86daaa7524c220980dd 
> 
> Diff: https://reviews.apache.org/r/49736/diff/
> 
> 
> Testing
> ---
> 
> $ cmake ..
> $ make -j
> $ make -j check
> 
> Manually inspected: `3rdparty/elfio-3.1/src/elfio-3.1/elfio/elfio_note.hpp`
> and saw that the patch is applied.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated July 7, 2016, 2:20 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Added test results from running benchmark on HEAD and with the patch.


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


Repository: mesos


Description
---

This parameterized test has the following configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 

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


Testing (updated)
---

All tests passed. Following results show that the support of shared resources 
has a small impact on runtime performance in allocations between HEAD and this 
patch. Also, there is no visible impact in performance when shared resources 
are added in the tests.

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported):
round 0 allocate took 3.074583secs to make 200 offers
round 50 allocate took 3.124076secs to make 200 offers
round 100 allocate took 3.136869secs to make 200 offers
round 150 allocate took 3.081407secs to make 200 offers
round 199 allocate took 3.087184secs to make 200 offers

With the patch (and no shared resources):
round 0 allocate took 3.297495secs to make 200 offers
round 50 allocate took 3.318641secs to make 200 offers
round 100 allocate took 3.312251secs to make 200 offers
round 150 allocate took 3.318052secs to make 200 offers
round 199 allocate took 3.3061secs to make 200 offers

With the patch (and shared resources on all agents):
round 0 allocate took 3.370602secs to make 200 offers
round 50 allocate took 3.380281secs to make 200 offers
round 100 allocate took 3.369976secs to make 200 offers
round 150 allocate took 3.372334secs to make 200 offers
round 199 allocate took 3.372049secs to make 200 offers

With the patch (and shared resources on alternate agents):
round 0 allocate took 3.360305secs to make 200 offers
round 50 allocate took 3.370903secs to make 200 offers
round 100 allocate took 3.369496secs to make 200 offers
round 150 allocate took 3.363246secs to make 200 offers
round 199 allocate took 3.485207secs to make 200 offers


Thanks,

Anindya Sinha

Re: Review Request 49446: Implemented LIST_FILES Call in v1 master API.

[zhou xing]

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




src/master/http.cpp (line 3143)


I think should be 4 spaces? as it's breaking one single line to two.


- zhou xing


On 七月 6, 2016, 4:51 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49446/
> ---
> 
> (Updated 七月 6, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5487
> https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented LIST_FILES Call in v1 master API.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
>   include/mesos/v1/master/master.proto 
> 19dbd1a19a2c4875d698f24be28bacea29d65821 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
>   src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 
> 
> Diff: https://reviews.apache.org/r/49446/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49731: Supported image volume rootfs mounted at container_path/rootfs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49731]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 6, 2016, 11:56 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49731/
> ---
> 
> (Updated July 6, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Timothy Chen, and Jiang 
> Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported image volume rootfs mounted at container_path/rootfs.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> db3ed8f69de8b52633194b252b0e5aba38ec69c0 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 4d852ad6b14934ae7ec691cc124c77bda9bb2c8d 
> 
> Diff: https://reviews.apache.org/r/49731/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.

[Jie Yu]

> On July 6, 2016, 11:50 p.m., Jie Yu wrote:
> > Can you verify with Docker code that `docker inspect` for image actually 
> > return the v1 image manifest? Better to add a link to the comments.
> 
> Kevin Klues wrote:
> Docker inspect seems to return its own type which is a superset of the 
> fields listed in the v1 image manifest spec:
> 
> Function to do the inspect:
> 
> https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/daemon/image_inspect.go
> 
> ImageInspect Type:
> 
> https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go
> 
> There doesn't seem to be any sort of "inheritance" going on where the 
> results of a `docker store` and the results of a `docker inspect` return 
> types that are based on on another.  Probably part of the reason the two 
> outputs differ in terms of how they case the labels.
> 
> Jie Yu wrote:
> Looks like some fields use the same type, but yeah, there is no 
> inheritence.
> 
> https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/image/image.go#L21
> 
> https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go#L116
> 
> Maybe we should model container.Config in our protobuf since this is 
> shared:
> 
> https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/container/config.go#L36
> 
> and then, module ImageInspect result as well in src/docker/spec.proto?
> 
> Kevin Klues wrote:
> That seems reasonable. The only field we specifically care about is 
> `Config.Labels`, so we could even think about passing just the `config` into 
> the `shouldInject()` function instead of a Manifest.

Yeah, that makes sense! there are also some tech debts i realized in 
`include/mesos/docker/*.proto`. We probably should split the proto message into 
namespaces, following docker:
```
docker.spec.container (docker.spec.container.Config)
docker.spec.engine (docker.spec.engine.ImageInspect)
docker.spec.image.v1
docker.spec.image.v2
```

Let's read the daemon code more to have a proper modeling.


- Jie


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


On July 6, 2016, 11:15 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49598/
> ---
> 
> (Updated July 6, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.
> 
> 
> Bugs: MESOS-5779
> https://issues.apache.org/jira/browse/MESOS-5779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `docker::spec::v1::ImageManifest` protobuf implements the
> official v1 image manifest specification found at:
> 
> https://github.com/docker/docker/blob/master/image/spec/v1.md
> 
> The field names in this spec are all written in snake_case as are the
> field names of the JSON representing the image manifest when reading
> it from disk (for example after performing a `docker save`). As such,
> the protobuf for ImageManifest also provides these fields in
> snake_case. Unfortunately, the `docker inspect` command also provides
> a method of retrieving the JSON for an image manifest, with one major
> caveat -- it represents all of its top level keys in CamelCase.
> 
> To allow both representations to be parsed in the same way, we
> intercept the incoming JSON from either source (disk or `docker
> inspect`) and convert it to a canonical snake_case representation.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49598/diff/
> 
> 
> Testing
> ---
> 
> `sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.

[Kevin Klues]

> On July 6, 2016, 11:50 p.m., Jie Yu wrote:
> > Can you verify with Docker code that `docker inspect` for image actually 
> > return the v1 image manifest? Better to add a link to the comments.
> 
> Kevin Klues wrote:
> Docker inspect seems to return its own type which is a superset of the 
> fields listed in the v1 image manifest spec:
> 
> Function to do the inspect:
> 
> https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/daemon/image_inspect.go
> 
> ImageInspect Type:
> 
> https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go
> 
> There doesn't seem to be any sort of "inheritance" going on where the 
> results of a `docker store` and the results of a `docker inspect` return 
> types that are based on on another.  Probably part of the reason the two 
> outputs differ in terms of how they case the labels.
> 
> Jie Yu wrote:
> Looks like some fields use the same type, but yeah, there is no 
> inheritence.
> 
> https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/image/image.go#L21
> 
> https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go#L116
> 
> Maybe we should model container.Config in our protobuf since this is 
> shared:
> 
> https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/container/config.go#L36
> 
> and then, module ImageInspect result as well in src/docker/spec.proto?

That seems reasonable. The only field we specifically care about is 
`Config.Labels`, so we could even think about passing just the `config` into 
the `shouldInject()` function instead of a Manifest.


- Kevin


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


On July 6, 2016, 11:15 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49598/
> ---
> 
> (Updated July 6, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.
> 
> 
> Bugs: MESOS-5779
> https://issues.apache.org/jira/browse/MESOS-5779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `docker::spec::v1::ImageManifest` protobuf implements the
> official v1 image manifest specification found at:
> 
> https://github.com/docker/docker/blob/master/image/spec/v1.md
> 
> The field names in this spec are all written in snake_case as are the
> field names of the JSON representing the image manifest when reading
> it from disk (for example after performing a `docker save`). As such,
> the protobuf for ImageManifest also provides these fields in
> snake_case. Unfortunately, the `docker inspect` command also provides
> a method of retrieving the JSON for an image manifest, with one major
> caveat -- it represents all of its top level keys in CamelCase.
> 
> To allow both representations to be parsed in the same way, we
> intercept the incoming JSON from either source (disk or `docker
> inspect`) and convert it to a canonical snake_case representation.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49598/diff/
> 
> 
> Testing
> ---
> 
> `sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49730: Added user doc for v1 master and agent API.

[Anand Mazumdar]

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


Fix it, then Ship it!





docs/operator-http-api.md (line 13)


s/url/URL


- Anand Mazumdar


On July 7, 2016, 12:27 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49730/
> ---
> 
> (Updated July 7, 2016, 12:27 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-4514
> https://issues.apache.org/jira/browse/MESOS-4514
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This doc doesn't include all the calls supported by the APIs yet.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49730/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/vinodkone/4695c9ab2718865ff3661d485f0eed3c
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.

[Jie Yu]

> On July 6, 2016, 11:50 p.m., Jie Yu wrote:
> > Can you verify with Docker code that `docker inspect` for image actually 
> > return the v1 image manifest? Better to add a link to the comments.
> 
> Kevin Klues wrote:
> Docker inspect seems to return its own type which is a superset of the 
> fields listed in the v1 image manifest spec:
> 
> Function to do the inspect:
> 
> https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/daemon/image_inspect.go
> 
> ImageInspect Type:
> 
> https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go
> 
> There doesn't seem to be any sort of "inheritance" going on where the 
> results of a `docker store` and the results of a `docker inspect` return 
> types that are based on on another.  Probably part of the reason the two 
> outputs differ in terms of how they case the labels.

Looks like some fields use the same type, but yeah, there is no inheritence.
https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/image/image.go#L21
https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go#L116

Maybe we should model container.Config in our protobuf since this is shared:
https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/container/config.go#L36

and then, module ImageInspect result as well in src/docker/spec.proto?


- Jie


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


On July 6, 2016, 11:15 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49598/
> ---
> 
> (Updated July 6, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.
> 
> 
> Bugs: MESOS-5779
> https://issues.apache.org/jira/browse/MESOS-5779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `docker::spec::v1::ImageManifest` protobuf implements the
> official v1 image manifest specification found at:
> 
> https://github.com/docker/docker/blob/master/image/spec/v1.md
> 
> The field names in this spec are all written in snake_case as are the
> field names of the JSON representing the image manifest when reading
> it from disk (for example after performing a `docker save`). As such,
> the protobuf for ImageManifest also provides these fields in
> snake_case. Unfortunately, the `docker inspect` command also provides
> a method of retrieving the JSON for an image manifest, with one major
> caveat -- it represents all of its top level keys in CamelCase.
> 
> To allow both representations to be parsed in the same way, we
> intercept the incoming JSON from either source (disk or `docker
> inspect`) and convert it to a canonical snake_case representation.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49598/diff/
> 
> 
> Testing
> ---
> 
> `sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49730: Added user doc for v1 master and agent API.

[Vinod Kone]

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

(Updated July 7, 2016, 12:27 a.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


Changes
---

anand's comments.


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


Repository: mesos


Description
---

This doc doesn't include all the calls supported by the APIs yet.


Diffs (updated)
-

  docs/operator-http-api.md PRE-CREATION 

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


Testing
---

https://gist.github.com/vinodkone/4695c9ab2718865ff3661d485f0eed3c


Thanks,

Vinod Kone

Review Request 49736: Added PATCH_CMD in 3rdparty/CMakeLists.txt for ELFIO.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Added PATCH_CMD in 3rdparty/CMakeLists.txt for ELFIO.


Diffs
-

  3rdparty/CMakeLists.txt a7ae76a40fcf5c6d6c9dc86daaa7524c220980dd 

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


Testing
---

$ cmake ..
$ make -j
$ make -j check

Manually inspected: `3rdparty/elfio-3.1/src/elfio-3.1/elfio/elfio_note.hpp`
and saw that the patch is applied.


Thanks,

Kevin Klues

Re: Review Request 49731: Supported image volume rootfs mounted at container_path/rootfs.

[Gilbert Song]

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

(Updated July 6, 2016, 4:56 p.m.)


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


Repository: mesos


Description
---

Supported image volume rootfs mounted at container_path/rootfs.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
db3ed8f69de8b52633194b252b0e5aba38ec69c0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
4d852ad6b14934ae7ec691cc124c77bda9bb2c8d 

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


Testing
---

make check

sudo ./bin/mesos-tests.sh


Thanks,

Gilbert Song

Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.

[Jie Yu]

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



Can you verify with Docker code that `docker inspect` for image actually return 
the v1 image manifest? Better to add a link to the comments.

- Jie Yu


On July 6, 2016, 11:15 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49598/
> ---
> 
> (Updated July 6, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.
> 
> 
> Bugs: MESOS-5779
> https://issues.apache.org/jira/browse/MESOS-5779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `docker::spec::v1::ImageManifest` protobuf implements the
> official v1 image manifest specification found at:
> 
> https://github.com/docker/docker/blob/master/image/spec/v1.md
> 
> The field names in this spec are all written in snake_case as are the
> field names of the JSON representing the image manifest when reading
> it from disk (for example after performing a `docker save`). As such,
> the protobuf for ImageManifest also provides these fields in
> snake_case. Unfortunately, the `docker inspect` command also provides
> a method of retrieving the JSON for an image manifest, with one major
> caveat -- it represents all of its top level keys in CamelCase.
> 
> To allow both representations to be parsed in the same way, we
> intercept the incoming JSON from either source (disk or `docker
> inspect`) and convert it to a canonical snake_case representation.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49598/diff/
> 
> 
> Testing
> ---
> 
> `sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49448: Added testcases for LIST_FILES call.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49448, 49551, 49550, 49447, 49446, 49529, 49696, 49445, 
49444, 49443, 49301]

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

Error:
2016-07-06 23:49:05 URL:https://reviews.apache.org/r/49445/diff/raw/ 
[9674/9674] -> "49445.patch" [1]
error: patch failed: src/files/files.hpp:17
error: src/files/files.hpp: patch does not apply
error: patch failed: src/files/files.cpp:53
error: src/files/files.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/14189/console

- Mesos ReviewBot


On July 6, 2016, 4:42 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49448/
> ---
> 
> (Updated July 6, 2016, 4:42 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5514
> https://issues.apache.org/jira/browse/MESOS-5514
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added testcases for LIST_FILES call.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7cf716dd7a5def8d648b1d8dbc2b9d5c158b00e9 
> 
> Diff: https://reviews.apache.org/r/49448/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo 
> GTEST_FILTER="*AgentAPITest.ListFiles*:FilesTest.BrowseTest:*MasterAPITest.ListFiles*"
>  make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49730: Added user doc for v1 master and agent API.

[Anand Mazumdar]

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



Looks good overall! Nothing major (just minor typos) + some comments around 
some details that are not required for a user doc.


docs/operator-http-api.md (line 8)


s/V1/v1

to be consistent with the other docs.



docs/operator-http-api.md (line 13)


Kill the second sentence?

It seems odd to have the details about the choice of endpoints (`/api/v1` vs
`/api/v1/operator`) in a user doc.



docs/operator-http-api.md (lines 15 - 17)


hmm, we already included the the endpoint details in the last line, no? Why 
do
it again here?

We don't do so in the scheduler/executor docs either.



docs/operator-http-api.md (line 19)


1. s/Following the same pattern as the/Similar to the

2. comma after HTTP APIs

3. s/we limit all all interactions with this API through/the endpoints only
accept HTTP POST

4. It's a bit unintuitive that the doc talks about `Calls` directly without 
explaining what they are?



docs/operator-http-api.md (line 21)


1. s/that must be present in the request/present in the request.

It's not mandatory for the header to be present. Should we also mention 
that we default to JSON if no accept header is present?

2. s/header **Accept-Encoding**/**Accept-Encoding** header

3. s/200/200 OK



docs/operator-http-api.md (line 23)


s/202/202 Accepted

s/gzip compression/Currently, gzip



docs/operator-http-api.md (line 25)


1. comma after future

Also, wondering if we should include this in the user doc?



docs/operator-http-api.md (line 29)


s/technically//
s/for those/for them



docs/operator-http-api.md (line 33)


s/technically//
s/for those/for them



docs/operator-http-api.md (line 34)


Can we include a Calls section similar to what we do for the 
scheduler/executor docs?



docs/operator-http-api.md (line 107)


s/!/.



docs/operator-http-api.md (line 142)


It's a bit confusing to see the "possibly compressed" line. Can we remove 
it since we don't support it currently?



docs/operator-http-api.md (line 144)


Can we put these in a "Events" section?



docs/operator-http-api.md (line 148)


s/See the above/See `SUBSCRIBE` above
s/state result/state can result
s/other events/more events



docs/operator-http-api.md (line 152)


s/This event is sent/This can happen



docs/operator-http-api.md (line 183)


s/This event is typically sent/This can happen


- Anand Mazumdar


On July 6, 2016, 10:46 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49730/
> ---
> 
> (Updated July 6, 2016, 10:46 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-4514
> https://issues.apache.org/jira/browse/MESOS-4514
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This doc doesn't include all the calls supported by the APIs yet.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49730/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/vinodkone/4695c9ab2718865ff3661d485f0eed3c
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 49616: Add suppression benchmark.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (lines 3572 - 3575)


We should use the templatized test fixture.

s/TEST_F/TEST_P/ and use `GetParam()`.



src/tests/hierarchical_allocator_tests.cpp (line 3587)


We actually don't need this count and it's misleading: it's not the count 
of offers. It's the count of frameworks that are receiving offers in this 
allocation round.

To get the offer count we can use `offers.size()`.



src/tests/hierarchical_allocator_tests.cpp (lines 3609 - 3616)


We can simply:

```
cout << "Using " << slaveCount << " agents and "
 << frameworkCount << " frameworks" << endl;

vector slaves(slaveCount);
vector frameworks(frameworkCount);
```



src/tests/hierarchical_allocator_tests.cpp (line 3648)


Nit: `s/count/i/`, we don't seem to use `count` in any special way and `i` 
is the standard variable name in a for loop.



src/tests/hierarchical_allocator_tests.cpp (lines 3650 - 3651)


Comment:

// Recover resources with no filters because we want to test the effect of 
suppression alone.



src/tests/hierarchical_allocator_tests.cpp (line 3654)


Comment:

// Wait for all declined offers to be processed so the stopwatch only 
measures the allocation time.



src/tests/hierarchical_allocator_tests.cpp (line 3660)


This `{` shouldn't be necessary.



src/tests/hierarchical_allocator_tests.cpp (line 3669)


Instead of calling it a `round` we could be more explicit:

"allocate() took 1.491969secs to make 2000 offers with 103 out of 200 
frameworks suppressing offers"


- Jiang Yan Xu


On July 5, 2016, 2:48 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49616/
> ---
> 
> (Updated July 5, 2016, 2:48 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5781
> https://issues.apache.org/jira/browse/MESOS-5781
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Useful for high framework clusters which carry
>   many suppressed frameworks that are considered
>   during allocation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49616/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.

[Kevin Klues]

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

(Updated July 6, 2016, 11:15 p.m.)


Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.


Changes
---

Updated based on comments.


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


Repository: mesos


Description
---

The `docker::spec::v1::ImageManifest` protobuf implements the
official v1 image manifest specification found at:

https://github.com/docker/docker/blob/master/image/spec/v1.md

The field names in this spec are all written in snake_case as are the
field names of the JSON representing the image manifest when reading
it from disk (for example after performing a `docker save`). As such,
the protobuf for ImageManifest also provides these fields in
snake_case. Unfortunately, the `docker inspect` command also provides
a method of retrieving the JSON for an image manifest, with one major
caveat -- it represents all of its top level keys in CamelCase.

To allow both representations to be parsed in the same way, we
intercept the incoming JSON from either source (disk or `docker
inspect`) and convert it to a canonical snake_case representation.


Diffs (updated)
-

  src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
  src/tests/containerizer/docker_tests.cpp 
00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 

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


Testing
---

`sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`


Thanks,

Kevin Klues

Re: Review Request 49694: Filter out fully used agents before allocate resources.

[Guangya Liu]

> On 七月 6, 2016, 9:17 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1220-1228
> > 
> >
> > I think the intention of this filtering was that the allocation loop 
> > needs to ignore agents that are not whitelisted or that are not activated, 
> > because otherwise it would send offers for them.
> > 
> > If the agent doesn't have free resources, that's the intention of 
> > checking `!allocatable(resources)` when we're trying to offer resources 
> > below. Technically, you should be checking the same here.
> > 
> > I'm confused by this patch since you showed that it doesn't help 
> > performance and this isn't necessary. Why did you send it?

Hi Ben, I did more test with 4000, 8000, 12000 fully used agent and 2000 
non-fully used agent, the test result showed that there are indeed some 
performnace improvement with the number of fully used agent increase.

1) 6000 agents (4000 fully used + 2000 non-fully used)

Before fix:
Using 6000 agents and 200 frameworks with 4000 agents fully used
round 0 allocate took 3.224707secs to make 200 offers
round 1 allocate took 3.205996secs to make 200 offers
round 2 allocate took 3.203761secs to make 200 offers
round 3 allocate took 3.520678secs to make 200 offers
round 4 allocate took 3.599001secs to make 200 offers
round 5 allocate took 3.437631secs to make 200 offers
round 6 allocate took 3.197855secs to make 200 offers
round 7 allocate took 3.210213secs to make 200 offers
round 8 allocate took 3.482421secs to make 200 offers
round 9 allocate took 3.495999secs to make 200 offers
round 10 allocate took 3.270022secs to make 200 offers
round 11 allocate took 3.303073secs to make 200 offers
round 12 allocate took 3.574657secs to make 200 offers
round 13 allocate took 3.20638secs to make 200 offers
round 14 allocate took 3.644309secs to make 200 offers
round 15 allocate took 3.22866secs to make 200 offers
round 16 allocate took 3.234343secs to make 200 offers
round 17 allocate took 3.457045secs to make 200 offers
round 18 allocate took 3.30394secs to make 200 offers
round 19 allocate took 3.323927secs to make 200 offers
round 20 allocate took 3.477471secs to make 200 offers
round 21 allocate took 3.494607secs to make 200 offers
round 22 allocate took 3.330647secs to make 200 offers


After Fix:
[ RUN  ] HierarchicalAllocator_BENCHMARK_Test.FilterOutFullyUsedAgent
Using 6000 agents and 200 frameworks with 4000 agents fully used
round 0 allocate took 3.101395secs to make 200 offers
round 1 allocate took 3.076541secs to make 200 offers
round 2 allocate took 3.054527secs to make 200 offers
round 3 allocate took 3.089352secs to make 200 offers
round 4 allocate took 3.670608secs to make 200 offers
round 5 allocate took 3.343736secs to make 200 offers
round 6 allocate took 3.268496secs to make 200 offers
round 7 allocate took 3.313199secs to make 200 offers
round 8 allocate took 3.269126secs to make 200 offers
round 9 allocate took 3.219127secs to make 200 offers
round 10 allocate took 3.259214secs to make 200 offers
round 11 allocate took 3.825867secs to make 200 offers
round 12 allocate took 3.907938secs to make 200 offers
round 13 allocate took 3.380813secs to make 200 offers
round 14 allocate took 3.116237secs to make 200 offers
round 15 allocate took 3.140145secs to make 200 offers
round 16 allocate took 3.059795secs to make 200 offers
round 17 allocate took 3.164924secs to make 200 offers
round 18 allocate took 3.424397secs to make 200 offers
round 19 allocate took 3.12893secs to make 200 offers
round 20 allocate took 3.101884secs to make 200 offers

2) 1 agents (8000 fully used + 2000 non-fully used)

Before Fix:
Using 1 agents and 200 frameworks with 8000 agents fully used
round 0 allocate took 3.372285secs to make 200 offers
round 1 allocate took 3.411058secs to make 200 offers
round 2 allocate took 3.3929secs to make 200 offers
round 3 allocate took 3.412163secs to make 200 offers
round 4 allocate took 3.434624secs to make 200 offers
round 5 allocate took 3.774675secs to make 200 offers
round 6 allocate took 3.570071secs to make 200 offers
round 7 allocate took 3.400986secs to make 200 offers
round 8 allocate took 3.438448secs to make 200 offers
round 9 allocate took 3.407503secs to make 200 offers
round 10 allocate took 3.463708secs to make 200 offers
round 11 allocate took 3.990451secs to make 200 offers
round 12 allocate took 4.181017secs to make 200 offers
round 13 allocate took 3.7594secs to make 200 offers
round 14 allocate took 3.76082secs to make 200 offers
round 15 allocate took 3.933429secs to make 200 offers
round 16 allocate took 3.559281secs to make 200 offers
round 17 allocate took 3.45657secs to make 200 offers
round 18 allocate took 3.5156secs to make 200 offers
round 19 allocate took 3.448579secs to make 200 offers
round 20 allocate took 3.517771secs to make 200 offers

After: Fix:
Using 1 agents and 200 

Review Request 49731: Supported image volume rootfs mounted at container_path/rootfs.

[Gilbert Song]

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

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


Repository: mesos


Description
---

Supported image volume rootfs mounted at container_path/rootfs.


Diffs
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
db3ed8f69de8b52633194b252b0e5aba38ec69c0 

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


Testing
---

make check

sudo ./bin/mesos-tests.sh


Thanks,

Gilbert Song

Re: Review Request 49543: Fix ProcessRemoteLinkTests that try to emulate 'stale' sockets.

[Benjamin Mahler]

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


Fix it, then Ship it!




You also need to disable the tests in this patch now, yes?


3rdparty/libprocess/src/tests/test_linkee.cpp (line 38)


Ditto here, can you use a pointer?



3rdparty/libprocess/src/tests/test_linkee.cpp (lines 40 - 41)


"Leak" seems a bit misleading, perhaps just say 'sockets' and comment that 
we never close these in order to leave them open?



3rdparty/libprocess/src/tests/test_linkee.cpp (line 41)


No static non-PODs, can you use a pointer and 'new' it?



3rdparty/libprocess/src/tests/test_linkee.cpp (line 103)


s/:/: /
s/construct/create/ here and below for outgoing



3rdparty/libprocess/src/tests/test_linkee.cpp (line 151)


The future may be discarded here as well, often we just use a tenary to 
print "discarded" for the discarded case.



3rdparty/libprocess/src/tests/test_linkee.cpp (line 160)


You shouldn't need to do this since Try allows non-const access: 
`outgoing->send`



3rdparty/libprocess/src/tests/test_linkee.cpp (line 166)


How about os::sleep here to be a bit more readable:

```
os::sleep(Seconds(1))
```


- Benjamin Mahler


On July 1, 2016, 11:53 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49543/
> ---
> 
> (Updated July 1, 2016, 11:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-5759
> https://issues.apache.org/jira/browse/MESOS-5759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The ProcessRemoteLinkTests `RemoteUseStaleLink` and 
> `RemoteStaleLinkRelink` were failing because the `test-linkee` was
> closing sockets shortly after the test `::shutdown` the link.  This 
> is the expected behavior of libprocess.
> 
> Rewrites the `test-linkee` to keep all sockets open, regardless of the
> state of the connection.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 7ba0f4833a5163d2323e1477509fd2fbacc23690 
> 
> Diff: https://reviews.apache.org/r/49543/diff/
> 
> 
> Testing
> ---
> 
> make check (CentOS 7)
> 
> 3rdparty/libprocess/libprocess-tests --gtest_filter="ProcessRemoteLinkTest*" 
> --gtest_break_on_failure --gtest_repeat=1
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49730: Added user doc for v1 master and agent API.

[Vinod Kone]

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

(Updated July 6, 2016, 10:46 p.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


Changes
---

zhitao's comments.


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


Repository: mesos


Description
---

This doc doesn't include all the calls supported by the APIs yet.


Diffs (updated)
-

  docs/operator-http-api.md PRE-CREATION 

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


Testing
---

https://gist.github.com/vinodkone/4695c9ab2718865ff3661d485f0eed3c


Thanks,

Vinod Kone

Re: Review Request 49730: Added user doc for v1 master and agent API.

[Zhitao Li]

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


Fix it, then Ship it!





docs/operator-http-api.md (line 19)


Do you want to document whether gzip support is available, and how to use 
it?



docs/operator-http-api.md (line 21)


Can you elaborate in what circumstances 202 or 200 would be returned?



docs/operator-http-api.md (line 105)


My impression is that we might also add event streaming for agent in the 
longer run.

Fine if we omit this until the actual work has started.


- Zhitao Li


On July 6, 2016, 9:42 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49730/
> ---
> 
> (Updated July 6, 2016, 9:42 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-4514
> https://issues.apache.org/jira/browse/MESOS-4514
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This doc doesn't include all the calls supported by the APIs yet.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49730/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/vinodkone/4695c9ab2718865ff3661d485f0eed3c
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 49722: Refactor all `get*` helper functions for master API to blocking version.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 6, 2016, 9:18 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49722/
> ---
> 
> (Updated July 6, 2016, 9:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This makes sure `GetState()` collection is done in an
> atomic fashion and avoid a race condition.
> 
> Things can be improved later:
> - `Swap` and RVO to avoid large data copy;
> - refactor the common logic of `ObjectApprover` into another helper function;
> - craft out test case for actual race condition.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 32b15d1f786c9ddbbf020b270f458b37ecea1bbd 
>   src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 
> 
> Diff: https://reviews.apache.org/r/49722/diff/
> 
> 
> Testing
> ---
> 
> `make check` for existing tests.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 49730: Added user doc for v1 master and agent API.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Zhitao Li.


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


Repository: mesos


Description
---

This doc doesn't include all the calls supported by the APIs yet.


Diffs
-

  docs/operator-http-api.md PRE-CREATION 

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


Testing
---

https://gist.github.com/vinodkone/4695c9ab2718865ff3661d485f0eed3c


Thanks,

Vinod Kone

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Gilbert Song]

> On July 6, 2016, 10:47 a.m., Gilbert Song wrote:
> > @yan: would you mind updating the unit test `DestroyWhileFetching` with 
> > another following patch?
> > 
> > seems like it does not capture the race correctly. it seems insufficient to 
> > me after the containerizer.detroy() call.
> 
> Jiang Yan Xu wrote:
> Will do.
> 
> Jiang Yan Xu wrote:
> Fixed in /r/49726/ 
> 
> Note that this test is only at the containeizer level. Megha has a 
> separate integration test which verifies the task state transitioning (to 
> TASK_FAILED).

SGTM! Thanks!


- Gilbert


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


On July 6, 2016, 2:20 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49726: Fixed 'DestroyWhileFetching' test.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On July 6, 2016, 2:32 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49726/
> ---
> 
> (Updated July 6, 2016, 2:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To verify that the container launch fails due to container destroy.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 57588cc1fb918924a163bdb40b195cc5f4231f6e 
> 
> Diff: https://reviews.apache.org/r/49726/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Note that this is still an approximation of what we should MESOS-5763 with. 
> Will have an integration test for it too.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49242: Add ReadFile protobuf message.

[Anand Mazumdar]

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




include/mesos/agent/agent.proto (lines 149 - 152)


Similar comment to previous review. Kill this and include the comments 
explaining the fields inline in the protobuf.

The reasoning is that, we are explaining the _business logic_ of the 
response and not the various arguments that were provided for the request. 
Those are _already_ explained in the request protobuf.

```cpp
// Contains the file data.
message ReadFile {
// Comment
optional string field1 = 1;
}
```

or

```cpp
// Contains the file data.
message ReadFile {
optional string field1 = 1; // Comment
}
```

Pick any style based on the size of the comment.



include/mesos/agent/agent.proto (line 152)


The 'length' field is part of the request. We need to mention about 'data' 
(part of response)


- Anand Mazumdar


On July 6, 2016, 5:19 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49242/
> ---
> 
> (Updated July 6, 2016, 5:19 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: mesos-5515
> https://issues.apache.org/jira/browse/mesos-5515
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ReadFile message in the Response message of master.proto
> , v1/master.proto, agent.proto, v1/agent.proto.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 538d12f71df1943f91bafb99650625aa910affaa 
>   include/mesos/master/master.proto d06258e9fd39c7eefd8ecd394e3bdfb888479b1e 
>   include/mesos/v1/agent/agent.proto 48f15173fe62b9ce7f648f6b54d74ec62f797c55 
>   include/mesos/v1/master/master.proto 
> b7cb6fdf2e4f34a11d326ac3ad3ec26525f8f343 
> 
> Diff: https://reviews.apache.org/r/49242/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 49725: Fail container launch if it's destroyed during logger->prepare().

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On July 6, 2016, 2:22 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49725/
> ---
> 
> (Updated July 6, 2016, 2:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fail container launch if it's destroyed during logger->prepare().
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49725/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49725: Fail container launch if it's destroyed during logger->prepare().

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 6, 2016, 9:22 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49725/
> ---
> 
> (Updated July 6, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fail container launch if it's destroyed during logger->prepare().
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49725/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49652: Improved Mesos containerizer invariant checking.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 6, 2016, 9:23 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49652/
> ---
> 
> (Updated July 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> One of the reasons for MESOS-5763 is due to the lack invariant 
> checking. Mesos containerizer transitions the container state in 
> particular ways so when continuation chains could potentially be
> interleaved with other actions we should verify the state transitions.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49652/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49652: Improved Mesos containerizer invariant checking.

[Jiang Yan Xu]

> On July 6, 2016, 11:45 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1058
> > 
> >
> > Should we check for DESTROYING here?

It was put in a later review but now I have moved it over to /r/49650/


> On July 6, 2016, 11:45 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1354-1356
> > 
> >
> > In fact, we should check for `contains` and `DESTROYING` as well 
> > because the container logger stuff made isolate async

Yeah you are right, fixed in /r/49725/ preceding this one (which is about 
invariants).


- Jiang Yan


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


On July 6, 2016, 2:23 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49652/
> ---
> 
> (Updated July 6, 2016, 2:23 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> One of the reasons for MESOS-5763 is due to the lack invariant 
> checking. Mesos containerizer transitions the container state in 
> particular ways so when continuation chains could potentially be
> interleaved with other actions we should verify the state transitions.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49652/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 49726: Fixed 'DestroyWhileFetching' test.

[Jiang Yan Xu]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

To verify that the container launch fails due to container destroy.


Diffs
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
57588cc1fb918924a163bdb40b195cc5f4231f6e 

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


Testing
---

make check.

Note that this is still an approximation of what we should MESOS-5763 with. 
Will have an integration test for it too.


Thanks,

Jiang Yan Xu

Re: Review Request 49653: Made Mesos containerizer error messages more consistent.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 6, 2016, 9:24 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49653/
> ---
> 
> (Updated July 6, 2016, 9:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We've been using slightly different wordings of the same condition in
> multiple places in Mesos containerizer but they don't provide
> additional information about where this failure is thrown in a long
> continuation chain. Since failures don't capture the location in the
> code we'd better distinguish them in a more meaningful way to assist
> debugging.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49653/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49679: Updated v1 operator Call::ReadFile message.

[Anand Mazumdar]

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




include/mesos/agent/agent.proto (lines 79 - 80)


How about just having this top level comment to explain the business logic 
of the call? You can then explain each of the fields as comments instead of 
doing it _all at once_ as a top level comment?

```cpp
// Reads data from a file.
message ReadFile {
// Comments
required string path = 1;

// Comments
optional string offset = 2;
}
```

See the `FileInfo` message for more info. Also, each of the field comments 
can be borrowed from `src/files/files.cpp` `read()` route handler.


- Anand Mazumdar


On July 6, 2016, 5:19 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49679/
> ---
> 
> (Updated July 6, 2016, 5:19 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: mesos-5515
> https://issues.apache.org/jira/browse/mesos-5515
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the type of offset and length of Call::ReadFile
> for v1 operator API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 538d12f71df1943f91bafb99650625aa910affaa 
>   include/mesos/master/master.proto d06258e9fd39c7eefd8ecd394e3bdfb888479b1e 
>   include/mesos/v1/agent/agent.proto 48f15173fe62b9ce7f648f6b54d74ec62f797c55 
>   include/mesos/v1/master/master.proto 
> b7cb6fdf2e4f34a11d326ac3ad3ec26525f8f343 
> 
> Diff: https://reviews.apache.org/r/49679/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 49694: Filter out fully used agents before allocate resources.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49694]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 6, 2016, 2:53 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49694/
> ---
> 
> (Updated July 6, 2016, 2:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5017
> https://issues.apache.org/jira/browse/MESOS-5017
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filter out fully used agents before allocate resources.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c1e00039606164599e25ff5f76245e4d35ec3112 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49694/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> I found that the performance actually does not have too much difference with 
> this patch.
> 
> Without this patch:
> Using 4000 agents and 200 frameworks with 2000 agents fully used
> round 0 allocate took 3.077414secs to make 200 offers
> round 1 allocate took 3.201985secs to make 200 offers
> round 2 allocate took 3.375356secs to make 200 offers
> round 3 allocate took 3.357918secs to make 200 offers
> round 4 allocate took 3.253266secs to make 200 offers
> round 5 allocate took 3.09918secs to make 200 offers
> round 6 allocate took 3.298442secs to make 200 offers
> round 7 allocate took 3.323613secs to make 200 offers
> round 8 allocate took 3.072355secs to make 200 offers
> round 9 allocate took 3.271617secs to make 200 offers
> round 10 allocate took 3.126364secs to make 200 offers
> round 11 allocate took 3.118216secs to make 200 offers
> round 12 allocate took 3.088154secs to make 200 offers
> round 13 allocate took 3.215559secs to make 200 offers
> round 14 allocate took 3.251781secs to make 200 offers
> round 15 allocate took 3.168862secs to make 200 offers
> 
> With this patch:
> Using 4000 agents and 200 frameworks with 2000 agents fully used
> round 0 allocate took 3.536161secs to make 200 offers
> round 1 allocate took 3.061345secs to make 200 offers
> round 2 allocate took 3.061557secs to make 200 offers
> round 3 allocate took 3.12996secs to make 200 offers
> round 4 allocate took 3.124199secs to make 200 offers
> round 5 allocate took 3.05091secs to make 200 offers
> round 6 allocate took 3.072797secs to make 200 offers
> round 7 allocate took 3.167616secs to make 200 offers
> round 8 allocate took 3.259008secs to make 200 offers
> round 9 allocate took 3.2203secs to make 200 offers
> round 10 allocate took 3.244719secs to make 200 offers
> round 11 allocate took 3.258939secs to make 200 offers
> round 12 allocate took 3.225309secs to make 200 offers
> round 13 allocate took 3.094871secs to make 200 offers
> round 14 allocate took 3.27408secs to make 200 offers
> round 15 allocate took 3.143926secs to make 200 offers
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49653: Made Mesos containerizer error messages more consistent.

[Jiang Yan Xu]

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

(Updated July 6, 2016, 2:24 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


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


Repository: mesos


Description
---

We've been using slightly different wordings of the same condition in
multiple places in Mesos containerizer but they don't provide
additional information about where this failure is thrown in a long
continuation chain. Since failures don't capture the location in the
code we'd better distinguish them in a more meaningful way to assist
debugging.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 49653: Made Mesos containerizer error messages more consistent.

[Jiang Yan Xu]

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

(Updated July 6, 2016, 2:23 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

We've been using slightly different wordings of the same condition in
multiple places in Mesos containerizer but they don't provide
additional information about where this failure is thrown in a long
continuation chain. Since failures don't capture the location in the
code we'd better distinguish them in a more meaningful way to assist
debugging.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 49652: Improved Mesos containerizer invariant checking.

[Jiang Yan Xu]

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

(Updated July 6, 2016, 2:23 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Rebased. Review comments are address in separate reviews.


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


Repository: mesos


Description
---

One of the reasons for MESOS-5763 is due to the lack invariant 
checking. Mesos containerizer transitions the container state in 
particular ways so when continuation chains could potentially be
interleaved with other actions we should verify the state transitions.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 49651: Improved Mesos containerizer logging and documentation.

[Jiang Yan Xu]

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

(Updated July 6, 2016, 2:22 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Improved Mesos containerizer logging and documentation.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 49725: Fail container launch if it's destroyed during logger->prepare().

[Jiang Yan Xu]

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

Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

Fail container launch if it's destroyed during logger->prepare().


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 49725: Fail container launch if it's destroyed during logger->prepare().

[Jiang Yan Xu]

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

(Updated July 6, 2016, 2:22 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Fail container launch if it's destroyed during logger->prepare().


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Jiang Yan Xu]

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

(Updated July 6, 2016, 2:20 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

If the container state is not properly set to FETCHING, Mesos agent
cannot detect the terminated executor when the fetcher times out.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.

Also with an experimental setup using mesos-execute with an agent with a fake 
hadoop binary that sleeps forever. The task is transitioned to LOST if the 
executor fetching times out; without the patch the task is stuck in STAGING.

Megha will submit a code test for this soon.


Thanks,

Jiang Yan Xu

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Jiang Yan Xu]

> On July 6, 2016, 10:47 a.m., Gilbert Song wrote:
> > @yan: would you mind updating the unit test `DestroyWhileFetching` with 
> > another following patch?
> > 
> > seems like it does not capture the race correctly. it seems insufficient to 
> > me after the containerizer.detroy() call.
> 
> Jiang Yan Xu wrote:
> Will do.

Fixed in /r/49726/ 

Note that this test is only at the containeizer level. Megha has a separate 
integration test which verifies the task state transitioning (to TASK_FAILED).


- Jiang Yan


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


On July 6, 2016, 10:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 10:07 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49722: Refactor all `get*` helper functions for master API to blocking version.

[Zhitao Li]

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

(Updated July 6, 2016, 9:18 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Repository: mesos


Description (updated)
---

This makes sure `GetState()` collection is done in an
atomic fashion and avoid a race condition.

Things can be improved later:
- `Swap` and RVO to avoid large data copy;
- refactor the common logic of `ObjectApprover` into another helper function;
- craft out test case for actual race condition.


Diffs
-

  src/master/http.cpp 32b15d1f786c9ddbbf020b270f458b37ecea1bbd 
  src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 

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


Testing
---

`make check` for existing tests.


Thanks,

Zhitao Li

Re: Review Request 49694: Filter out fully used agents before allocate resources.

[Benjamin Mahler]

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




src/master/allocator/mesos/hierarchical.cpp (lines 1220 - 1228)


I think the intention of this filtering was that the allocation loop needs 
to ignore agents that are not whitelisted or that are not activated, because 
otherwise it would send offers for them.

If the agent doesn't have free resources, that's the intention of checking 
`!allocatable(resources)` when we're trying to offer resources below. 
Technically, you should be checking the same here.

I'm confused by this patch since you showed that it doesn't help 
performance and this isn't necessary. Why did you send it?


- Benjamin Mahler


On July 6, 2016, 2:53 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49694/
> ---
> 
> (Updated July 6, 2016, 2:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5017
> https://issues.apache.org/jira/browse/MESOS-5017
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filter out fully used agents before allocate resources.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c1e00039606164599e25ff5f76245e4d35ec3112 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49694/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> I found that the performance actually does not have too much difference with 
> this patch.
> 
> Without this patch:
> Using 4000 agents and 200 frameworks with 2000 agents fully used
> round 0 allocate took 3.077414secs to make 200 offers
> round 1 allocate took 3.201985secs to make 200 offers
> round 2 allocate took 3.375356secs to make 200 offers
> round 3 allocate took 3.357918secs to make 200 offers
> round 4 allocate took 3.253266secs to make 200 offers
> round 5 allocate took 3.09918secs to make 200 offers
> round 6 allocate took 3.298442secs to make 200 offers
> round 7 allocate took 3.323613secs to make 200 offers
> round 8 allocate took 3.072355secs to make 200 offers
> round 9 allocate took 3.271617secs to make 200 offers
> round 10 allocate took 3.126364secs to make 200 offers
> round 11 allocate took 3.118216secs to make 200 offers
> round 12 allocate took 3.088154secs to make 200 offers
> round 13 allocate took 3.215559secs to make 200 offers
> round 14 allocate took 3.251781secs to make 200 offers
> round 15 allocate took 3.168862secs to make 200 offers
> 
> With this patch:
> Using 4000 agents and 200 frameworks with 2000 agents fully used
> round 0 allocate took 3.536161secs to make 200 offers
> round 1 allocate took 3.061345secs to make 200 offers
> round 2 allocate took 3.061557secs to make 200 offers
> round 3 allocate took 3.12996secs to make 200 offers
> round 4 allocate took 3.124199secs to make 200 offers
> round 5 allocate took 3.05091secs to make 200 offers
> round 6 allocate took 3.072797secs to make 200 offers
> round 7 allocate took 3.167616secs to make 200 offers
> round 8 allocate took 3.259008secs to make 200 offers
> round 9 allocate took 3.2203secs to make 200 offers
> round 10 allocate took 3.244719secs to make 200 offers
> round 11 allocate took 3.258939secs to make 200 offers
> round 12 allocate took 3.225309secs to make 200 offers
> round 13 allocate took 3.094871secs to make 200 offers
> round 14 allocate took 3.27408secs to make 200 offers
> round 15 allocate took 3.143926secs to make 200 offers
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good, just some minor comments below.


src/docker/spec.cpp (lines 212 - 232)


I was a bit surprised to see that the `"config"` field's keys use 
CamelCase. Maybe a more explicit note, or a parentetical example of `"config"` 
fields being CamelCase?

Seems we should more explicitly spell out to the reader that we 
**intentionally** do not recurse when doing the transformation due to this? The 
only hint right now seems to be from:

"it represents all of its **top level** keys in CamelCase." (emphasis mine)

Which only tells me that we don't have to care about recursing, but it 
doesn't tell me that we **must not** recurse (because e.g. "config" uses 
CamelCase).



src/docker/spec.cpp (lines 239 - 257)


How about reducing the levels of nesting to make this a bit easier to read?

```
  foreachpair (const string& key, const JSON::Value& value, _json.values) {
if (key == "Size") {
  continue; // "Size" is the only exception!
}

for (unsigned int i = 0; i < key.length(); ++i) {
  ...
}
...
  }
```



src/docker/spec.cpp (lines 242 - 251)


Using a foreach seems a bit simpler?

```
foreach (char c, camelKey) {
  if (std::isupper(c)) {
if (!snakeKey.empty()) {
  snakeKey += "_";
}
snakeKey += std::tolower(c);
  } else {
snakeKey += c;
  }
}
```

More explicitly naming them camelKey and snakeKey seems a bit easier on the 
reader?



src/tests/containerizer/docker_tests.cpp (lines 672 - 673)


Shouldn't this be an INTERNET test? (Do you also need CURL?)



src/tests/containerizer/docker_tests.cpp (lines 736 - 739)


Thanks for the comment. Can you also add a TODO here to do a complete 
comparision?



src/tests/containerizer/docker_tests.cpp (lines 741 - 743)


EXPECT_EQ?


- Benjamin Mahler


On July 4, 2016, 5:58 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49598/
> ---
> 
> (Updated July 4, 2016, 5:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.
> 
> 
> Bugs: MESOS-5779
> https://issues.apache.org/jira/browse/MESOS-5779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `docker::spec::v1::ImageManifest` protobuf implements the
> official v1 image manifest specification found at:
> 
> https://github.com/docker/docker/blob/master/image/spec/v1.md
> 
> The field names in this spec are all written in snake_case as are the
> field names of the JSON representing the image manifest when reading
> it from disk (for example after performing a `docker save`). As such,
> the protobuf for ImageManifest also provides these fields in
> snake_case. Unfortunately, the `docker inspect` command also provides
> a method of retrieving the JSON for an image manifest, with one major
> caveat -- it represents all of its top level keys in CamelCase.
> 
> To allow both representations to be parsed in the same way, we
> intercept the incoming JSON from either source (disk or `docker
> inspect`) and convert it to a canonical snake_case representation.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
>   src/tests/containerizer/docker_tests.cpp 
> 49bd7c252c342c8cb29ea916ad3f1f71638d2017 
> 
> Diff: https://reviews.apache.org/r/49598/diff/
> 
> 
> Testing
> ---
> 
> `sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49722: Refactor all `get*` helper functions for master API to blocking version.

[Zhitao Li]

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

(Updated July 6, 2016, 9 p.m.)


Review request for mesos.


Changes
---

Add helper `http::subscribe()` and rebase.


Repository: mesos


Description (updated)
---

This makes sure `GetState()` collection is done in an
atomic fashion and avoid a race condition.

Things can be improved later:
- `Swap` and RVO to avoid large data.
- craft out test case for actual race condition.


Diffs (updated)
-

  src/master/http.cpp 32b15d1f786c9ddbbf020b270f458b37ecea1bbd 
  src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 

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


Testing
---

`make check` for existing tests.


Thanks,

Zhitao Li

Re: Review Request 49448: Added testcases for LIST_FILES call.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/tests/api_tests.cpp (line 785)


s/ListFilesSuccessful/ListFiles



src/tests/api_tests.cpp (lines 821 - 822)


How about:
// This test verifies that the client will receive a `NotFound` response 
when it tries to make a `LIST_FILES` call with an invalid path.

Ditto for the agent test.



src/tests/api_tests.cpp (line 823)


ListFilesUnsuccessful/ListFilesInvalidPath



src/tests/api_tests.cpp (lines 827 - 836)


Do you need these for this test?


- Anand Mazumdar


On July 6, 2016, 4:42 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49448/
> ---
> 
> (Updated July 6, 2016, 4:42 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5514
> https://issues.apache.org/jira/browse/MESOS-5514
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added testcases for LIST_FILES call.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7cf716dd7a5def8d648b1d8dbc2b9d5c158b00e9 
> 
> Diff: https://reviews.apache.org/r/49448/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo 
> GTEST_FILTER="*AgentAPITest.ListFiles*:FilesTest.BrowseTest:*MasterAPITest.ListFiles*"
>  make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Jiang Yan Xu]

> On July 6, 2016, 10:47 a.m., Gilbert Song wrote:
> > @yan: would you mind updating the unit test `DestroyWhileFetching` with 
> > another following patch?
> > 
> > seems like it does not capture the race correctly. it seems insufficient to 
> > me after the containerizer.detroy() call.

Will do.


- Jiang Yan


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


On July 6, 2016, 10:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 10:07 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49446: Implemented LIST_FILES Call in v1 master API.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/master/http.cpp (line 3143)


2 space indent before `->`


- Anand Mazumdar


On July 6, 2016, 4:51 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49446/
> ---
> 
> (Updated July 6, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5487
> https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented LIST_FILES Call in v1 master API.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
>   include/mesos/v1/master/master.proto 
> 19dbd1a19a2c4875d698f24be28bacea29d65821 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
>   src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 
> 
> Diff: https://reviews.apache.org/r/49446/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49696: Used alias for the namespace 'process::http'.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/files/files.cpp (line 62)


s/processHttp/http

We have used the alias name as `http` in other files. Would be good to be 
consistent throughout. Did it not work for you?


- Anand Mazumdar


On July 6, 2016, 7:51 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49696/
> ---
> 
> (Updated July 6, 2016, 7:51 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, zhou xing, haosdent huang, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5487
> https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used alias for the namespace 'process::http'.
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 0368f67fc184dbd29945fcb9216439f0a5b713bd 
> 
> Diff: https://reviews.apache.org/r/49696/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49445: Updated FilesProcess to support List_Files Call in Operator API v1.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/files/files.cpp (lines 356 - 358)


Do you need the `defer` here anymore?


- Anand Mazumdar


On July 6, 2016, 11:33 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49445/
> ---
> 
> (Updated July 6, 2016, 11:33 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5487
> https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated FilesProcess to support List_Files Call in Operator API v1.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 
>   src/files/files.cpp a5a1b86e14f63e5e3834a2900270252fbe16f586 
>   src/tests/files_tests.cpp 31337e280c6224a8c949c8868a53e5a785b4573f 
> 
> Diff: https://reviews.apache.org/r/49445/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49308: Added agent and scheduler authentication backoff.

[Adam B]

> On July 6, 2016, 11:21 a.m., Adam B wrote:
> > src/sched/sched.cpp, line 483
> > 
> >
> > Seems `failedAuthentications` is never 0 here (since you increment it 
> > just before), so you'll never delay by the `[0, b * 2^0]` amount you 
> > suggest in the docs. Should we make this `std::pow(2, 
> > failedAuthentications-1)` or update the doc?
> > Same issue on the agent.

I just updated the doc.
We also discussed adding an initial delay before authenticating, to prevent 
thundering herds. I added TODOs to that effect.


- Adam


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


On July 6, 2016, 6:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49308/
> ---
> 
> (Updated July 6, 2016, 6:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-2043
> https://issues.apache.org/jira/browse/MESOS-2043
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The backoff follows to existing pattern for backoff used during agent
> or scheduler registration where we backoff for some random time in an
> interval of increasing length, capped by
> `REGISTER_RETRY_INTERVAL_MAX`.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 8c8678c7e2251923298b90b7216a4e584faf6b26 
>   docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
>   docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
>   src/sched/constants.hpp df8a1cc83ee3986400d633b2192b6da7fbe6b626 
>   src/sched/flags.hpp 989cebe40c6b4ecc7c4d47f8cf9d968cc795ad3f 
>   src/sched/sched.cpp 9f561d73a2e591afdc3ba4adb35a11763dced402 
>   src/slave/constants.hpp 668fc47e72d6f1b904aef5d0750b990fe162c9a3 
>   src/slave/flags.hpp ff45876a44ed00fdea36986f052f10e8b8031925 
>   src/slave/flags.cpp 010e78347f72edd5e60628b8bdda8de8b5feed21 
>   src/slave/slave.hpp 484ba758b4c87935aabd2f76a0e654a3c6d09167 
>   src/slave/slave.cpp 36f63bc54bec88f7e7b11ed0cde8bc78314908b2 
> 
> Diff: https://reviews.apache.org/r/49308/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X w/o optimizations).
> 
> Ran agent-related `AuthenticationTest`s in repetition (300 times).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 49696: Used alias for the namespace 'process::http'.

[Abhishek Dasgupta]

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

(Updated July 6, 2016, 7:51 p.m.)


Review request for mesos, Adam B, Anand Mazumdar, zhou xing, haosdent huang, 
and Vinod Kone.


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


Repository: mesos


Description
---

Used alias for the namespace 'process::http'.


Diffs (updated)
-

  src/files/files.cpp 0368f67fc184dbd29945fcb9216439f0a5b713bd 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49279: Added documentation around available client libraries.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 6, 2016, 7:07 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49279/
> ---
> 
> (Updated July 6, 2016, 7:07 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5419
> https://issues.apache.org/jira/browse/MESOS-5419
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds documentation for supported/user contributed
> client libraries available for the Mesos HTTP API.
> 
> 
> Diffs
> -
> 
>   docs/api-client-libraries.md PRE-CREATION 
>   docs/home.md 2d728333a3c1421b310d861b71c92691fd41a482 
> 
> Diff: https://reviews.apache.org/r/49279/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/hatred/9e3f8909dae814465df5f560874e0a98
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49722: Refactor all `get*` helper functions for master API to blocking version.

[Zhitao Li]

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

(Updated July 6, 2016, 7:17 p.m.)


Review request for mesos.


Repository: mesos


Description (updated)
---

WIP.

This makes sure `GetState` collection is done in an
atomic fasion.

Things can be improved later:
1. Refactor the code for handling `Subscribe`;
2. `Swap` and RVO for zero copy for large data;
3. Refactor the repeated code of creating the `Owned` approver 
objects.


Diffs
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
  src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 

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


Testing
---

`make check` for existing tests.


Thanks,

Zhitao Li

Re: Review Request 49722: Refactor all `get*` helper functions for master API to blocking version.

[Zhitao Li]

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

(Updated July 6, 2016, 7:16 p.m.)


Review request for mesos.


Repository: mesos


Description
---

WIP.

This makes sure `GetState` collection is done in an
atomic fasion.

Things can be improved later:
1. Refactor the code for handling `Subscribe`;
2. `Swap` and RVO for zero copy for large data.


Diffs
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
  src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 

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


Testing (updated)
---

`make check` for existing tests.


Thanks,

Zhitao Li

Review Request 49722: Refactor all `get*` helper functions for master API to blocking version.

[Zhitao Li]

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

Review request for mesos.


Repository: mesos


Description
---

WIP.

This makes sure `GetState` collection is done in an
atomic fasion.

Things can be improved later:
1. Refactor the code for handling `Subscribe`;
2. `Swap` and RVO for zero copy for large data.


Diffs
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
  src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 49279: Added documentation around available client libraries.

[Anand Mazumdar]

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

(Updated July 6, 2016, 7:07 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

There were some rendering issues with the version of Markdown used in our 
website around displaying hyperlinks for table. I updated the diff to get rid
of the `Repository` column and just have `Name/Language`. @vinodkone can you 
take a look again?


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


Repository: mesos


Description
---

This change adds documentation for supported/user contributed
client libraries available for the Mesos HTTP API.


Diffs (updated)
-

  docs/api-client-libraries.md PRE-CREATION 
  docs/home.md 2d728333a3c1421b310d861b71c92691fd41a482 

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


Testing
---

https://gist.github.com/hatred/9e3f8909dae814465df5f560874e0a98


Thanks,

Anand Mazumdar

Re: Review Request 49653: Made Mesos containerizer error messages more consistent.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (line 991)


I would be consistent and always mention the current state. So in this 
case, I would still say:

```
Container destroyed during provisioning
```



src/slave/containerizer/mesos/containerizer.cpp (line 998)


Ditto.



src/slave/containerizer/mesos/containerizer.cpp (line 1056)


Ditto. During isolating.



src/slave/containerizer/mesos/containerizer.cpp (line 1096)


Ditto. During preparing.



src/slave/containerizer/mesos/containerizer.cpp (line 1395)


Ditto. During fetching.


- Jie Yu


On July 6, 2016, 5:10 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49653/
> ---
> 
> (Updated July 6, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We've been using slightly different wordings of the same condition in
> multiple places in Mesos containerizer but they don't provide
> additional information about where this failure is thrown in a long
> continuation chain. Since failures don't capture the location in the
> code we'd better distinguish them in a more meaningful way to assist
> debugging.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49653/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49710: Updated CHANGELOG with more accurate blurb about Nvidia GPU support.

[Benjamin Mahler]

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


Fix it, then Ship it!




Thanks!


CHANGELOG (lines 106 - 110)


Two additional things that would be good to call out in this blurb:

* Frameworks must opt-in to receiving these resources via the GPU_RESOURCES 
capability.
* We support using nvidia-docker style images, via injecting a volume 
containing the Nvidia driver libraries and binaries.


- Benjamin Mahler


On July 6, 2016, 4:34 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49710/
> ---
> 
> (Updated July 6, 2016, 4:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CHANGELOG with more accurate blurb about Nvidia GPU support.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 16ee3efafb5646ef02d0433a668c93ccfe66e0e6 
> 
> Diff: https://reviews.apache.org/r/49710/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49652: Improved Mesos containerizer invariant checking.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1058)


Should we check for DESTROYING here?



src/slave/containerizer/mesos/containerizer.cpp (lines 1354 - 1356)


In fact, we should check for `contains` and `DESTROYING` as well because 
the container logger stuff made isolate async


- Jie Yu


On July 6, 2016, 5:09 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49652/
> ---
> 
> (Updated July 6, 2016, 5:09 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> One of the reasons for MESOS-5763 is due to the lack invariant 
> checking. Mesos containerizer transitions the container state in 
> particular ways so when continuation chains could potentially be
> interleaved with other actions we should verify the state transitions.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49652/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49600: Added authz to /files/debug endpoint.

[Abhishek Dasgupta]

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

(Updated July 6, 2016, 11:37 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Alexander Rojas, and Jan 
Schlicht.


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


Repository: mesos


Description
---

Added authz to /files/debug endpoint.


Diffs
-

  docs/authorization.md 002501c59d75c20fa91479077ef426346221ce89 
  docs/upgrades.md 255b5bd44c01bb7bfc8d9d7a11f393b2b2502084 
  src/common/http.cpp 0060121674421e3da16f4e3fdb1b2496787544dc 
  src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 
  src/files/files.cpp a5a1b86e14f63e5e3834a2900270252fbe16f586 
  src/master/main.cpp 84f3b0723fd3df02386c8072ded3cb42272cd065 
  src/slave/main.cpp a7ed669a0b6861d6ce8546dfafac849044a77eec 
  src/tests/files_tests.cpp 31337e280c6224a8c949c8868a53e5a785b4573f 

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


Testing
---

On Ubuntu 16.04:

sudo GTEST_FILTER="FilesTest.DebugTest" make -j4 check


Thanks,

Abhishek Dasgupta

Re: Review Request 49685: Added filtering for orphaned executors in `GET_EXECUTORS` operator API.

[Joerg Schad]

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


Fix it, then Ship it!





src/master/http.cpp (line 1557)


s/info/executorInfo ?


- Joerg Schad


On July 6, 2016, 6:07 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49685/
> ---
> 
> (Updated July 6, 2016, 6:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added filtering for orphaned executors in `GET_EXECUTORS` operator API.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
> 
> Diff: https://reviews.apache.org/r/49685/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49681: Renamed `unsubscribed_frameworks` to `recovered_frameworks`.

[Vinod Kone]

> On July 6, 2016, 6:30 p.m., Vinod Kone wrote:
> >

i'll fix this while committing.


- Vinod


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


On July 6, 2016, 6:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49681/
> ---
> 
> (Updated July 6, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed `unsubscribed_frameworks` to `recovered_frameworks`.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
>   include/mesos/v1/master/master.proto 
> 19dbd1a19a2c4875d698f24be28bacea29d65821 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
> 
> Diff: https://reviews.apache.org/r/49681/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49686: Added filtering for orphaned tasks in `GET_TASKS` operator API.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 6, 2016, 6:08 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49686/
> ---
> 
> (Updated July 6, 2016, 6:08 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added filtering for orphaned tasks in `GET_TASKS` operator API.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
> 
> Diff: https://reviews.apache.org/r/49686/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49685: Added filtering for orphaned executors in `GET_EXECUTORS` operator API.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 6, 2016, 6:07 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49685/
> ---
> 
> (Updated July 6, 2016, 6:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added filtering for orphaned executors in `GET_EXECUTORS` operator API.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
> 
> Diff: https://reviews.apache.org/r/49685/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49681: Renamed `unsubscribed_frameworks` to `recovered_frameworks`.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/http.cpp (line 1430)


s/orphaned/recovered/



src/master/http.cpp (line 1432)


s/orphaned/recovered/


- Vinod Kone


On July 6, 2016, 6:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49681/
> ---
> 
> (Updated July 6, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed `unsubscribed_frameworks` to `recovered_frameworks`.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
>   include/mesos/v1/master/master.proto 
> 19dbd1a19a2c4875d698f24be28bacea29d65821 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
> 
> Diff: https://reviews.apache.org/r/49681/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49651: Improved Mesos containerizer logging and documentation.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/containerizer.cpp (line 1575)


terminated fetcher sounds like a launch failure, should we just mention 
launch failure here?


- Jie Yu


On July 6, 2016, 5:08 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49651/
> ---
> 
> (Updated July 6, 2016, 5:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved Mesos containerizer logging and documentation.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49651/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49681: Renamed `unsubscribed_frameworks` to `recovered_frameworks`.

[Joerg Schad]

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


Ship it!




Ship It!

- Joerg Schad


On July 6, 2016, 6:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49681/
> ---
> 
> (Updated July 6, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed `unsubscribed_frameworks` to `recovered_frameworks`.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
>   include/mesos/v1/master/master.proto 
> 19dbd1a19a2c4875d698f24be28bacea29d65821 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
> 
> Diff: https://reviews.apache.org/r/49681/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (line 3629)


Looks like our test doesn't use `offerCount` at all?

If we keep `offerCount` we can log how many total offers are generated at 
the end of the test. It's not an essential metric but I guess it would be nice 
to confirm.



src/tests/hierarchical_allocator_tests.cpp (line 3671)


Strictly speaking this test doesn't need these used resources so we should 
explain a little bit.

```
// Add some used resources on each agent so it's more realistic in a 
framework failover scenario.
```



src/tests/hierarchical_allocator_tests.cpp (line 3677)


Someone sometime later still needs to do a sweep of the codebase for this 
but now that we are 1.0 let's start using the new terminology in places that 
are not confusing.

s/slave/agent/.



src/tests/hierarchical_allocator_tests.cpp (lines 3680 - 3685)


Move this below framework deactivation and add a comment:

```
// During framework failover all its offers are recovered.
```



src/tests/hierarchical_allocator_tests.cpp (lines 3682 - 3683)


We don't use this indentation format per the style guide.

How about just:

```
allocator->recoverResources(
offer.frameworkId,
offer.slaveId,
offer.resources,
None());
```



src/tests/hierarchical_allocator_tests.cpp (line 3685)


Use a blank line above.



src/tests/hierarchical_allocator_tests.cpp (line 3686)


No reset.



src/tests/hierarchical_allocator_tests.cpp (lines 3690 - 3691)


`deactivateFramework` would reset the effect of `suppressOffers` so this 
line is unnecessary.



src/tests/hierarchical_allocator_tests.cpp (line 3690)


No need to suppress here as we are failing over anyways.



src/tests/hierarchical_allocator_tests.cpp (line 3700)


s/resoures/resources/



src/tests/hierarchical_allocator_tests.cpp (lines 3700 - 3702)


Add:

This is to simulate the real world scenario where a large number of 
frameworks fail over simulatenously but the allocator processes framework 
activations in batches interleaved by offer declinations and suppressions from 
frameworks reregistered earlier.



src/tests/hierarchical_allocator_tests.cpp (line 3703)


```
TODO(jjanco): Parameterize the test by the batch size instead of using an 
arbitrary number.
```



src/tests/hierarchical_allocator_tests.cpp (line 3704)


Use `std::lround`?

This math here would be equivalent but it would be nice if we don't have to 
explain it. :)



src/tests/hierarchical_allocator_tests.cpp (line 3708)


Single space around `+=`.

More importantly, will this exclude the frameworks numbered after the last 
"full-sized" batch? i.e., framework 5-8 if frameworkCount == 9?

I suggest we restructure the loop as described below.



src/tests/hierarchical_allocator_tests.cpp (lines 3709 - 3710)


Add:

This is to simulate the behavior of non-greedy frameworks: after the 
failover they immediately decline and suppress offers until further work is 
requested.



src/tests/hierarchical_allocator_tests.cpp (lines 3711 - 3725)


1. We should recover resources after the whole batch is finished and not 
after each `activateFramework` is called, as this is often the case in 
realistic scenarios.
2. If we don't `settle()` then vector is actually not thread-safe so I 
think we need to settle here.

How about this for the entire loop?

```
  for (unsigned batchStart = 0;
   batchStart < frameworkCount;
   batchStart += batchSize) {
unsigned batchEnd = min(batchStart + batchSize, frameworkCount);
  
// Activate all frameworks in this batch to trigger allocations.
for (unsigned i = batchStart; i < batchEnd; i++) {
  allocator->activateFramework(frameworks[i].id());
}

// 

Re: Review Request 49308: Added agent and scheduler authentication backoff.

[Adam B]

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


Fix it, then Ship it!




Looks great, except for the mismatch between your math and the docs.


src/sched/sched.cpp (lines 479 - 480)


We generally prefer to wrap after the `=`, but I'll allow this since the 
rhs doesn't all fit on one line



docs/configuration.md (line 974)


s/authentication/authenticate/



docs/configuration.md (line 975)


s/1str/1st/



src/sched/flags.hpp (line 44)


Did you want a `\n` at the end of the line?



src/sched/flags.hpp (lines 45 - 46)


These two lines fit together. I'm not sure why you wrapped.



src/sched/sched.cpp (line 480)


Seems `failedAuthentications` is never 0 here (since you increment it just 
before), so you'll never delay by the `[0, b * 2^0]` amount you suggest in the 
docs. Should we make this `std::pow(2, failedAuthentications-1)` or update the 
doc?
Same issue on the agent.



src/slave/flags.cpp (line 241)


s/authentication/authenticate/



src/slave/flags.cpp (line 242)


s/1str/1st/


- Adam B


On July 6, 2016, 6:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49308/
> ---
> 
> (Updated July 6, 2016, 6:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-2043
> https://issues.apache.org/jira/browse/MESOS-2043
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The backoff follows to existing pattern for backoff used during agent
> or scheduler registration where we backoff for some random time in an
> interval of increasing length, capped by
> `REGISTER_RETRY_INTERVAL_MAX`.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 8c8678c7e2251923298b90b7216a4e584faf6b26 
>   docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
>   docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
>   src/sched/constants.hpp df8a1cc83ee3986400d633b2192b6da7fbe6b626 
>   src/sched/flags.hpp 989cebe40c6b4ecc7c4d47f8cf9d968cc795ad3f 
>   src/sched/sched.cpp 9f561d73a2e591afdc3ba4adb35a11763dced402 
>   src/slave/constants.hpp 668fc47e72d6f1b904aef5d0750b990fe162c9a3 
>   src/slave/flags.hpp ff45876a44ed00fdea36986f052f10e8b8031925 
>   src/slave/flags.cpp 010e78347f72edd5e60628b8bdda8de8b5feed21 
>   src/slave/slave.hpp 484ba758b4c87935aabd2f76a0e654a3c6d09167 
>   src/slave/slave.cpp 36f63bc54bec88f7e7b11ed0cde8bc78314908b2 
> 
> Diff: https://reviews.apache.org/r/49308/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X w/o optimizations).
> 
> Ran agent-related `AuthenticationTest`s in repetition (300 times).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 49686: Added filtering for orphaned tasks in `GET_TASKS` operator API.

[haosdent huang]

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

(Updated July 6, 2016, 6:08 p.m.)


Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
Li.


Changes
---

Rebase.


Repository: mesos


Description
---

Added filtering for orphaned tasks in `GET_TASKS` operator API.


Diffs (updated)
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49685: Added filtering for orphaned executors in `GET_EXECUTORS` operator API.

[haosdent huang]

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

(Updated July 6, 2016, 6:07 p.m.)


Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
Li.


Changes
---

Rebase.


Repository: mesos


Description
---

Added filtering for orphaned executors in `GET_EXECUTORS` operator API.


Diffs (updated)
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49681: Renamed `unsubscribed_frameworks` to `recovered_frameworks`.

[haosdent huang]

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

(Updated July 6, 2016, 6:06 p.m.)


Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
Li.


Changes
---

Fix compile errors.


Repository: mesos


Description
---

Renamed `unsubscribed_frameworks` to `recovered_frameworks`.


Diffs (updated)
-

  include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
  include/mesos/v1/master/master.proto 19dbd1a19a2c4875d698f24be28bacea29d65821 
  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49690: Supported docker config agent flag with invalid value.

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/docker/store.cpp (line 107)


Can we check explicitly using os::exists? We should error out if read fails.



src/slave/containerizer/mesos/provisioner/docker/store.cpp (lines 125 - 130)


We probably should return an Error here per BenM's comments.


- Jie Yu


On July 6, 2016, 7:30 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49690/
> ---
> 
> (Updated July 6, 2016, 7:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent flag 'docker_config' can be either a path of the docker
> config file or as a JSON Object. Currently, if the 'docker_config'
> is invalid, e.g., non-existed file or invalid JSON Object, the
> agent cannot start. This is not ideal because the agent should
> have many other tasks without interacting with docker private
> registries. We should just return error/failure while pulling a
> private repo without correct 'docker_config', instead of blocking
> the agent to start.
> 
> Secondly, the semantic of specifying 'docker_config' as a path is
> changed. The path used to be starting with 'file://', so the file
> can be automatically loaded/fetched from the agent flag. Now, in
> order to unblock the agent from an invalid 'docker_config', the
> path has to be an absolute path starting with '/', in which case
> the path will be read explicitly. However, if users still follow
> the old semantic, it should still work, because the 'docker_config'
> starting with 'file://' will be parsed as a string, then it would
> be regarded as a JSON Object case.
> 
> So the behavior is if the 'docker_config' is invalid, mesos will
> log a warning and continue to start. The task pulling a private
> repo without the correct docker config file will fail.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp f1ecf3b25d85597f6c3dcaa47968860ed119dbd5 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 81e9d94553874a74cc115d9a1fd659652313a94c 
>   src/slave/flags.hpp ff45876a44ed00fdea36986f052f10e8b8031925 
> 
> Diff: https://reviews.apache.org/r/49690/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> and tested manually with a private repo in both docker containerizer and 
> mesos containerizer:
> 
> sudo ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --docker_image=gilbertsong/inky --shell=true --command="env"
> 
> sudo ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --containerizer=docker --docker_image=gilbertsong/inky --shell=true 
> --command="env"
> 
> Verified that invalid docker_config (non-existed path or invalid JSON) will 
> still have the agent launch correctly, with a correct warning printed out.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49600: Added authz to /files/debug endpoint.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On July 6, 2016, 8:13 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49600/
> ---
> 
> (Updated July 6, 2016, 8:13 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Alexander Rojas, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-5369
> https://issues.apache.org/jira/browse/MESOS-5369
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authz to /files/debug endpoint.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 002501c59d75c20fa91479077ef426346221ce89 
>   docs/upgrades.md 255b5bd44c01bb7bfc8d9d7a11f393b2b2502084 
>   src/common/http.cpp 0060121674421e3da16f4e3fdb1b2496787544dc 
>   src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 
>   src/files/files.cpp a5a1b86e14f63e5e3834a2900270252fbe16f586 
>   src/master/main.cpp 84f3b0723fd3df02386c8072ded3cb42272cd065 
>   src/slave/main.cpp a7ed669a0b6861d6ce8546dfafac849044a77eec 
>   src/tests/files_tests.cpp 31337e280c6224a8c949c8868a53e5a785b4573f 
> 
> Diff: https://reviews.apache.org/r/49600/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> 
> sudo GTEST_FILTER="FilesTest.DebugTest" make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Gilbert Song]

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



@yan: would you mind updating the unit test `DestroyWhileFetching` with another 
following patch?

seems like it does not capture the race correctly. it seems insufficient to me 
after the containerizer.detroy() call.

- Gilbert Song


On July 6, 2016, 10:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 10:07 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 6, 2016, 5:07 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49700: Added test to check orphaned tasks are filtered.

[Joerg Schad]

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

(Updated July 6, 2016, 5:42 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added test to check orphaned tasks are filtered.


Diffs (updated)
-

  src/tests/master_authorization_tests.cpp 
21a65c47f972056ba3b5c9552a3a507da2a00124 

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


Testing
---

(sudo) make check on various platforms


Thanks,

Joerg Schad

Re: Review Request 49686: Added filtering for orphaned tasks in `GET_TASKS` operator API.

[haosdent huang]

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

(Updated July 6, 2016, 5:39 p.m.)


Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
Li.


Changes
---

Address comments.


Repository: mesos


Description
---

Added filtering for orphaned tasks in `GET_TASKS` operator API.


Diffs (updated)
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49685: Added filtering for orphaned executors in `GET_EXECUTORS` operator API.

[haosdent huang]

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

(Updated July 6, 2016, 5:39 p.m.)


Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
Li.


Changes
---

Address comments.


Repository: mesos


Description
---

Added filtering for orphaned executors in `GET_EXECUTORS` operator API.


Diffs (updated)
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49681: Renamed `unsubscribed_frameworks` to `recovered_frameworks`.

[haosdent huang]

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

(Updated July 6, 2016, 5:38 p.m.)


Review request for mesos, Anand Mazumdar, Joerg Schad, Vinod Kone, and Zhitao 
Li.


Changes
---

Address comments.


Repository: mesos


Description
---

Renamed `unsubscribed_frameworks` to `recovered_frameworks`.


Diffs (updated)
-

  include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
  include/mesos/v1/master/master.proto 19dbd1a19a2c4875d698f24be28bacea29d65821 
  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Gilbert Song]

> On July 6, 2016, 10:20 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1054
> > 
> >
> > Please add a check on `DESTROYING` state, otherwise it may cost a race 
> > if destroy while fetching.
> > 
> > ```
> >   if (containers_[containerId]->state == DESTROYING) {
> > return Failure("Container is currently being destroyed");
> >   }
> > ```
> 
> Jiang Yan Xu wrote:
> It's in /r/49653/, for this patch I wanted to focus on fixing what 
> directly broke the container destroy path when the fetcher is stuck.
> 
> Jiang Yan Xu wrote:
> Well, I guess I can move it over as well since /r/49653/ focuses on error 
> messages.

Thanks:)


- Gilbert


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


On July 6, 2016, 10:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 10:07 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Jiang Yan Xu]

> On July 6, 2016, 10:20 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1054
> > 
> >
> > Please add a check on `DESTROYING` state, otherwise it may cost a race 
> > if destroy while fetching.
> > 
> > ```
> >   if (containers_[containerId]->state == DESTROYING) {
> > return Failure("Container is currently being destroyed");
> >   }
> > ```
> 
> Jiang Yan Xu wrote:
> It's in /r/49653/, for this patch I wanted to focus on fixing what 
> directly broke the container destroy path when the fetcher is stuck.

Well, I guess I can move it over as well since /r/49653/ focuses on error 
messages.


- Jiang Yan


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


On July 6, 2016, 10:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 10:07 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Jiang Yan Xu]

> On July 6, 2016, 10:20 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1054
> > 
> >
> > Please add a check on `DESTROYING` state, otherwise it may cost a race 
> > if destroy while fetching.
> > 
> > ```
> >   if (containers_[containerId]->state == DESTROYING) {
> > return Failure("Container is currently being destroyed");
> >   }
> > ```

It's in /r/49653/, for this patch I wanted to focus on fixing what directly 
broke the container destroy path when the fetcher is stuck.


- Jiang Yan


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


On July 6, 2016, 10:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 10:07 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49697: Removed CHECK from orphan_task filtering.

[Joerg Schad]

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

(Updated July 6, 2016, 5:21 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This CHECK can be problematic when upgrading from old agents to
a new master.


Diffs (updated)
-

  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 

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


Testing
---

(sudo) make check on various platforms


Thanks,

Joerg Schad

Re: Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Gilbert Song]

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


Fix it, then Ship it!




Hmm...It surprises me the FETCHING state is not set for a long time. Thanks for 
fixing it.


src/slave/containerizer/mesos/containerizer.cpp (line 1054)


Please add a check on `DESTROYING` state, otherwise it may cost a race if 
destroy while fetching.

```
  if (containers_[containerId]->state == DESTROYING) {
return Failure("Container is currently being destroyed");
  }
```


- Gilbert Song


On July 6, 2016, 10:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49650/
> ---
> 
> (Updated July 6, 2016, 10:07 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-5763
> https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container state is not properly set to FETCHING, Mesos agent
> cannot detect the terminated executor when the fetcher times out.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 
> 
> Diff: https://reviews.apache.org/r/49650/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Also with an experimental setup using mesos-execute with an agent with a fake 
> hadoop binary that sleeps forever. The task is transitioned to LOST if the 
> executor fetching times out; without the patch the task is stuck in STAGING.
> 
> Megha will submit a code test for this soon.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 49653: Made Mesos containerizer error messages more consistent.

[Jiang Yan Xu]

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

Review request for mesos, Jie Yu and Vinod Kone.


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


Repository: mesos


Description
---

We've been using slightly different wordings of the same condition in
multiple places in Mesos containerizer but they don't provide
additional information about where this failure is thrown in a long
continuation chain. Since failures don't capture the location in the
code we'd better distinguish them in a more meaningful way to assist
debugging.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 49693: Cleaned up some minor style issues.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49692, 49693]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 6, 2016, 8:39 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49693/
> ---
> 
> (Updated July 6, 2016, 8:39 a.m.)
> 
> 
> Review request for mesos, Joerg Schad and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up some minor style issues.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49693/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49697: Removed CHECK from orphan_task filtering.

[haosdent huang]

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




src/master/http.cpp (line 2609)


I saw we use `frameworkId` in most places.


- haosdent huang


On July 6, 2016, 4:18 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49697/
> ---
> 
> (Updated July 6, 2016, 4:18 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5757
> https://issues.apache.org/jira/browse/MESOS-5757
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This CHECK can be problematic when upgrading from old agents to
> a new master.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
> 
> Diff: https://reviews.apache.org/r/49697/diff/
> 
> 
> Testing
> ---
> 
> (sudo) make check on various platforms
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 49651: Improved Mesos containerizer logging and documentation.

[Jiang Yan Xu]

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

Review request for mesos, Jie Yu and Vinod Kone.


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


Repository: mesos


Description
---

Improved Mesos containerizer logging and documentation.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 49652: Improved Mesos containerizer invariant checking.

[Jiang Yan Xu]

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

Review request for mesos, Jie Yu and Vinod Kone.


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


Repository: mesos


Description
---

One of the reasons for MESOS-5763 is due to the lack invariant 
checking. Mesos containerizer transitions the container state in 
particular ways so when continuation chains could potentially be
interleaved with other actions we should verify the state transitions.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 49650: Fixed Mesos containerizer to set container FETCHING state.

[Jiang Yan Xu]

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

Review request for mesos, Jie Yu and Vinod Kone.


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


Repository: mesos


Description
---

If the container state is not properly set to FETCHING, Mesos agent
cannot detect the terminated executor when the fetcher times out.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
f53b01b0eef8dd24db28d9dbd86bcbd40dc8d17f 

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


Testing
---

make check.

Also with an experimental setup using mesos-execute with an agent with a fake 
hadoop binary that sleeps forever. The task is transitioned to LOST if the 
executor fetching times out; without the patch the task is stuck in STAGING.

Megha will submit a code test for this soon.


Thanks,

Jiang Yan Xu

Re: Review Request 49690: Supported docker config agent flag with invalid value.

[Benjamin Mahler]

We almost never accept badly formed configuration, why allow it here?

On Wednesday, July 6, 2016, Jie Yu  wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49690/
>
> Can you also follow up with some documentation?
>
>
> - Jie Yu
>
> On July 6th, 2016, 7:30 a.m. UTC, Gilbert Song wrote:
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and
> Timothy Chen.
> By Gilbert Song.
>
> *Updated July 6, 2016, 7:30 a.m.*
> *Repository: * mesos
> Description
>
> The agent flag 'docker_config' can be either a path of the docker
> config file or as a JSON Object. Currently, if the 'docker_config'
> is invalid, e.g., non-existed file or invalid JSON Object, the
> agent cannot start. This is not ideal because the agent should
> have many other tasks without interacting with docker private
> registries. We should just return error/failure while pulling a
> private repo without correct 'docker_config', instead of blocking
> the agent to start.
>
> Secondly, the semantic of specifying 'docker_config' as a path is
> changed. The path used to be starting with 'file://', so the file
> can be automatically loaded/fetched from the agent flag. Now, in
> order to unblock the agent from an invalid 'docker_config', the
> path has to be an absolute path starting with '/', in which case
> the path will be read explicitly. However, if users still follow
> the old semantic, it should still work, because the 'docker_config'
> starting with 'file://' will be parsed as a string, then it would
> be regarded as a JSON Object case.
>
> So the behavior is if the 'docker_config' is invalid, mesos will
> log a warning and continue to start. The task pulling a private
> repo without the correct docker config file will fail.
>
> Testing
>
> make check
>
> and tested manually with a private repo in both docker containerizer and 
> mesos containerizer:
>
> sudo ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --docker_image=gilbertsong/inky --shell=true --command="env"
>
> sudo ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --containerizer=docker --docker_image=gilbertsong/inky --shell=true 
> --command="env"
>
> Verified that invalid docker_config (non-existed path or invalid JSON) will 
> still have the agent launch correctly, with a correct warning printed out.
>
> Diffs
>
>- src/slave/containerizer/docker.cpp
>(f1ecf3b25d85597f6c3dcaa47968860ed119dbd5)
>- src/slave/containerizer/mesos/provisioner/docker/store.cpp
>(81e9d94553874a74cc115d9a1fd659652313a94c)
>- src/slave/flags.hpp (ff45876a44ed00fdea36986f052f10e8b8031925)
>
> View Diff 
>

Re: Review Request 49447: Implemented LIST_FILES Call in v1 agent API.

[Abhishek Dasgupta]

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

(Updated July 6, 2016, 4:52 p.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Implemented LIST_FILES Call in v1 agent API.


Diffs (updated)
-

  include/mesos/agent/agent.proto 538d12f71df1943f91bafb99650625aa910affaa 
  include/mesos/v1/agent/agent.proto 48f15173fe62b9ce7f648f6b54d74ec62f797c55 
  src/slave/http.cpp ef2d510b86e5d4f731c2ea6b7df8246a0be9d812 
  src/slave/slave.hpp 484ba758b4c87935aabd2f76a0e654a3c6d09167 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49446: Implemented LIST_FILES Call in v1 master API.

[Abhishek Dasgupta]

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

(Updated July 6, 2016, 4:51 p.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Implemented LIST_FILES Call in v1 master API.


Diffs (updated)
-

  include/mesos/master/master.proto 6aedc2fe9aa1eaae7744ec20baf1c9cac766329a 
  include/mesos/v1/master/master.proto 19dbd1a19a2c4875d698f24be28bacea29d65821 
  src/master/http.cpp 3640486d02f523ee6eb7c40fe959f42260e4cd2d 
  src/master/master.hpp 0688ba33efcf670da6a6861870d1a13caad22b66 

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


Testing
---


Thanks,

Abhishek Dasgupta