Re: Review Request 63366: Added jemalloc release tarball and build rules.

[Benno Evers]

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

(Updated April 18, 2018, 4:15 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

Added jemalloc release tarball and build rules.


Diffs
-

  3rdparty/CMakeLists.txt 488e906486a583e74faceb44d906cee5036a8b99 
  3rdparty/Makefile.am 10b29c9e2d10073a817273c7038226d0ed4fd6ad 
  3rdparty/cmake/Versions.cmake 605cbdebe2518c234a55921dc07443546ec045e2 
  3rdparty/jemalloc-5.0.1.tar.bz2 PRE-CREATION 
  3rdparty/versions.am 63d879bd2c61503912a578a1c375d3d5c736656a 
  CHANGELOG c02d56d6612c432bb079a15fde84cb30d7455971 
  cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
  configure.ac 4c4085c50a8b3baa8c7b61da614b84ae218cf51c 
  src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
  src/master/CMakeLists.txt b8953bd576586ac8548a1b51cf0a87bc8e9da4fc 
  src/slave/CMakeLists.txt 943e8f523f867e3dd4030f78eca7830500578faf 


Diff: https://reviews.apache.org/r/63366/diff/9/


Testing
---


Thanks,

Benno Evers

Re: Review Request 63366: Added jemalloc release tarball and build rules.

[Alexander Rukletsov]

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


Fix it, then Ship it!




I'll fix the issues and commit shortly.


3rdparty/Makefile.am
Lines 140-153 (patched)


No need for this magic since we can simply repack in `.tar.gz`.


- Alexander Rukletsov


On April 18, 2018, 4:15 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63366/
> ---
> 
> (Updated April 18, 2018, 4:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7944
> https://issues.apache.org/jira/browse/MESOS-7944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added jemalloc release tarball and build rules.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 488e906486a583e74faceb44d906cee5036a8b99 
>   3rdparty/Makefile.am 10b29c9e2d10073a817273c7038226d0ed4fd6ad 
>   3rdparty/cmake/Versions.cmake 605cbdebe2518c234a55921dc07443546ec045e2 
>   3rdparty/jemalloc-5.0.1.tar.bz2 PRE-CREATION 
>   3rdparty/versions.am 63d879bd2c61503912a578a1c375d3d5c736656a 
>   CHANGELOG c02d56d6612c432bb079a15fde84cb30d7455971 
>   cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
>   configure.ac 4c4085c50a8b3baa8c7b61da614b84ae218cf51c 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/master/CMakeLists.txt b8953bd576586ac8548a1b51cf0a87bc8e9da4fc 
>   src/slave/CMakeLists.txt 943e8f523f867e3dd4030f78eca7830500578faf 
> 
> 
> Diff: https://reviews.apache.org/r/63366/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63368: Added MemoryProfiler class to Libprocess.

[Alexander Rukletsov]

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


Ship it!




I will commit this now, fixing outstanding typos and style issues. I will also 
refactor `DiskArtifact` to be more aligned with the single version of the 
artifact and refactor some functions to minimize taking `Try<>`s as parameters. 
I think we can come up with even cleaner implementation.

- Alexander Rukletsov


On April 18, 2018, 4:15 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63368/
> ---
> 
> (Updated April 18, 2018, 4:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7944
> https://issues.apache.org/jira/browse/MESOS-7944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class exposes profiling functionality of jemalloc
> memory allocator when it is detected to be the memory
> allocator of the current process.
> 
> In particular, it enables developers to access live
> statistics of current memory usage, and to create
> heap profiles that show where most memory is allocated.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c895d3ac7b9cc5ffd6c8b57ff24def866eb0213d 
>   3rdparty/libprocess/include/process/process.hpp 
> c36df991b6a2c120ab0562e8ff907f9fbf8630d1 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/memory_profiler.hpp PRE-CREATION 
>   3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> 9eb37465cd86f408d69f5f98fb76c4f4b93b9acd 
> 
> 
> Diff: https://reviews.apache.org/r/63368/diff/12/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63370: Added new --memory_profiling flag to agent and master binaries.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/slave/main.cpp
Lines 369 (patched)


Extra newline.


- Alexander Rukletsov


On April 18, 2018, 4:15 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63370/
> ---
> 
> (Updated April 18, 2018, 4:15 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7944
> https://issues.apache.org/jira/browse/MESOS-7944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag allows explicit disabling of the memory profiler
> endpoint in the master and agent binaries.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 505786e5631c891a52d9d7db403eff312f461d3d 
>   src/master/flags.cpp dbb35befd612f4be1019293c1889d19296118d07 
>   src/master/main.cpp 3def0f29e4d580306d2ecef033a37aad5333c8ea 
>   src/slave/flags.hpp beae47f0f8f2178b93a3484d168ce4d71c961841 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/main.cpp dcc944840d4e4e611e8ed1b0110b8f1bb189bde9 
> 
> 
> Diff: https://reviews.apache.org/r/63370/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63370: Added new --memory_profiling flag to agent and master binaries.

[Benno Evers]

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

(Updated April 18, 2018, 4:15 a.m.)


Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

This flag allows explicit disabling of the memory profiler
endpoint in the master and agent binaries.


Diffs
-

  src/master/flags.hpp 505786e5631c891a52d9d7db403eff312f461d3d 
  src/master/flags.cpp dbb35befd612f4be1019293c1889d19296118d07 
  src/master/main.cpp 3def0f29e4d580306d2ecef033a37aad5333c8ea 
  src/slave/flags.hpp beae47f0f8f2178b93a3484d168ce4d71c961841 
  src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
  src/slave/main.cpp dcc944840d4e4e611e8ed1b0110b8f1bb189bde9 


Diff: https://reviews.apache.org/r/63370/diff/7/


Testing
---


Thanks,

Benno Evers

Re: Review Request 63366: Added jemalloc release tarball and build rules.

[Benno Evers]

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

(Updated April 18, 2018, 4:15 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

Added jemalloc release tarball and build rules.


Diffs
-

  3rdparty/CMakeLists.txt 488e906486a583e74faceb44d906cee5036a8b99 
  3rdparty/Makefile.am 10b29c9e2d10073a817273c7038226d0ed4fd6ad 
  3rdparty/cmake/Versions.cmake 605cbdebe2518c234a55921dc07443546ec045e2 
  3rdparty/jemalloc-5.0.1.tar.bz2 PRE-CREATION 
  3rdparty/versions.am 63d879bd2c61503912a578a1c375d3d5c736656a 
  CHANGELOG c02d56d6612c432bb079a15fde84cb30d7455971 
  cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
  configure.ac 4c4085c50a8b3baa8c7b61da614b84ae218cf51c 
  src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
  src/master/CMakeLists.txt b8953bd576586ac8548a1b51cf0a87bc8e9da4fc 
  src/slave/CMakeLists.txt 943e8f523f867e3dd4030f78eca7830500578faf 


Diff: https://reviews.apache.org/r/63366/diff/9/


Testing
---


Thanks,

Benno Evers

Re: Review Request 63368: Added MemoryProfiler class to Libprocess.

[Benno Evers]

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

(Updated April 18, 2018, 4:15 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

This class exposes profiling functionality of jemalloc
memory allocator when it is detected to be the memory
allocator of the current process.

In particular, it enables developers to access live
statistics of current memory usage, and to create
heap profiles that show where most memory is allocated.


Diffs
-

  3rdparty/libprocess/Makefile.am c895d3ac7b9cc5ffd6c8b57ff24def866eb0213d 
  3rdparty/libprocess/include/process/process.hpp 
c36df991b6a2c120ab0562e8ff907f9fbf8630d1 
  3rdparty/libprocess/src/CMakeLists.txt 
0ce7dac5deea94623530820d173ce3ffe5b42ea4 
  3rdparty/libprocess/src/memory_profiler.hpp PRE-CREATION 
  3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp 9eb37465cd86f408d69f5f98fb76c4f4b93b9acd 


Diff: https://reviews.apache.org/r/63368/diff/12/


Testing
---


Thanks,

Benno Evers

Re: Review Request 66671: Updated composing containerizer tests. (WIP)

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [8, 9, 66670, 66671]

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

- Mesos Reviewbot


On April 17, 2018, 3:23 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66671/
> ---
> 
> (Updated April 17, 2018, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8737
> https://issues.apache.org/jira/browse/MESOS-8737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates composing containerizer tests in order to be
> consistent with the unification of `destroy()` and `wait()` return
> types.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 6964ac2f050396a733b04e650e59d6c29c59665d 
> 
> 
> Diff: https://reviews.apache.org/r/66671/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

[Chun-Hung Hsiao]

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




src/slave/slave.hpp
Line 815 (original), 819 (patched)


This is inconsistent with the existing codebase. Could you justify why 
favoring `unique_ptr` over `Owned`?



src/slave/slave.cpp
Line 4606 (original), 4601-4616 (patched)


How about the following?
```
Result resourceProviderId =
  getResourceProviderId(operation->info());

CHECK(!resourceProviderId.isError());

if (CHECKresourceProviderId.isSome()) {
  CHECK_NOTNULL(resourceProviderManager.get())
->acknowedgeOperationStatus(acknowledgement);
}
```
I prefer this way to encapsulate the implementation details about how to 
get the resource provider ID from the consumed resources.



src/slave/slave.cpp
Lines 8177-8179 (patched)


Let's check that all resources are agent default resources.



src/slave/slave.cpp
Line 8187 (original), 8201 (patched)


```
CHECK_NOTNULL(resourceProviderManager.get())
  ->publishResources(resources);
```



src/slave/slave.cpp
Lines 8822 (patched)


`flags` is not used in this function.



src/slave/slave.cpp
Lines 8829 (patched)


Could you elaborate on this? Or make it a TODO if the comment is too 
complicated to be added ;)



src/slave/slave.cpp
Lines 8833-8840 (patched)


There was a recent discussion in the API WG about adding routes dynamically 
(after initialization). The discussion started with this ticket: 
https://issues.apache.org/jira/browse/MESOS-7697

In summary, libprocess would return a 404 if a route has not been added 
yet, but for certain endpoints (e.g., the `/api/v1/scheduler`, or the agent 
resource provider config API calls), 404 might also be used by the API handler 
to represent missing resources. A client would have no way to distinguish 
what's the meaning of a 404, and if it should retry.

Several ideas had been proposed, sech as:
(1) Establishing a convention that a Mesos API handler never uses 404, but 
use 410 instead. But the semantics of "GONE" dose not quite match "NOT FOUND".

(2) Make libprocess returns 503 in the beginning, and then at certain point 
of time, mark libprocess as "initialized", meaning that no more routes will be 
added, and after that libprocess could return 404 if a route is added.

Along with the discussion of (2), people seems to agree that in most cases 
dynamic addition of routes is not very useful and (2) seems a viable solution. 
There's no conclusion yet, but if we're going to follow (2), I'd avoid adding 
`/api/v1/resource_provider` here, but just registering this in 
`Slave::initialize()`, and let the resource provider manager rejects requests 
until it obtains the slave ID. This is also what I did for 
`LocalResourceProviderDaemon`: 
https://github.com/apache/mesos/blob/master/src/resource_provider/daemon.cpp#L147.

Before we establish the convention, I'd suggest that we avoid adding routes 
after `Slave::initialize()`. Thoughts?


- Chun-Hung Hsiao


On April 10, 2018, 12:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66308/
> ---
> 
> (Updated April 10, 2018, 12:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/3/
> 
> 
> Testing
> ---
> 
> `make 

Re: Review Request 66679: Made the master send operation status updates when dropping operations.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66679 was successfully built and tested.

Reviews applied: `['66679']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66679

- Mesos Reviewbot Windows


On April 18, 2018, 12:21 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66679/
> ---
> 
> (Updated April 18, 2018, 12:21 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8781
> https://issues.apache.org/jira/browse/MESOS-8781
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the master send an operation status update to the
> framework with  status `OPERATION_ERROR` when an operation with an
> operation ID is dropped.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66679/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66608: Improved support for move-only types in `hashmap`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66608]

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

- Mesos Reviewbot


On April 17, 2018, 7:35 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66608/
> ---
> 
> (Updated April 17, 2018, 7:35 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While it was already possible to create a `hashmap` over move-only
> values, we still performed a copy in `put`, making it hard to
> dynamically add elements with the expected stout semantics.
> 
> This patch relaxes the requirements on the value argument to `put` so
> that instead of copyable we now only require move-constructible types.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/hashmap.hpp 
> 91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 
> 
> 
> Diff: https://reviews.apache.org/r/66608/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66648: Marked the resource provider API as experimental.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On April 17, 2018, 12:22 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66648/
> ---
> 
> (Updated April 17, 2018, 12:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, Jie 
> Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8787
> https://issues.apache.org/jira/browse/MESOS-8787
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch marks the resource provider API as experimental, and make the
> v0 and v1 protos consistent.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> db7c751bb61fb1ee2421015dcbefc021c3afbdac 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 42bc050ed01a272603a41ab052ed75d799dd76e2 
> 
> 
> Diff: https://reviews.apache.org/r/66648/diff/2/
> 
> 
> Testing
> ---
> 
> No need for testing since only comments are changed.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66650: Removed an invalid TODO in puller.cpp.

[Qian Zhang]

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



Mind to explain why this TODO is invalid in the description? :-)

- Qian Zhang


On April 17, 2018, 1:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66650/
> ---
> 
> (Updated April 17, 2018, 1:32 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an invalid TODO in puller.cpp.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
> 
> 
> Diff: https://reviews.apache.org/r/66650/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66227: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API.

[Chun-Hung Hsiao]

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




src/tests/api_tests.cpp
Lines 3688 (patched)


Let's try to split the test into two so each test verifies one operation.

Also, what's the reason of disabling the test on Windows?

And do you think it is reasonable to have tests that leave `agent_id` 
unset, to expose the code path of validating the existence of `agent_id`?



src/tests/api_tests.cpp
Lines 3692-3695 (patched)


This is not needed. The overloaded `MasterAPITest::CreateMasterFlags()` 
already extends the allocation interval.



src/tests/api_tests.cpp
Lines 3700 (patched)


Since the API is not called XXXVolumes, let's just say
```
// ... use it in the API calls.
```
so need to break the line here.



src/tests/api_tests.cpp
Lines 3711-3719 (patched)


No need to add the `RESOURCE_PROVIDER` capability since this is set up by 
default in 1.6. In fact, we should remove it here so if the default 
capabilities are changed in the future, we will be able to capture the problem 
through this test.



src/tests/api_tests.cpp
Lines 3750 (patched)


Let's move this to the beginning of this test.



src/tests/api_tests.cpp
Lines 3766 (patched)


Not sure if this is necessary. Could you justify the need of this 
synchronization?



src/tests/api_tests.cpp
Lines 3768 (patched)


Is this necessary?



src/tests/api_tests.cpp
Lines 3787 (patched)


Do we need this? IIRC an event-based allocation will be triggered (and thus 
an offer will be sent) when a framework is added.



src/tests/api_tests.cpp
Lines 3791 (patched)


```
Offer offer = offersAfterCreate->at(0);
```



src/tests/api_tests.cpp
Lines 3801-3809 (patched)


Let's do the following for readability:
```
v1::master::Call::GrowVolume* growVolume =
  v1GrowVolumeCall.mutable_grow_volume();
growVolume->mutable_agent_id()->CopyFrom(evolve(slaveId));
growVolume->mutable_volume()->CopyFrom(evolve(volume));
growVolume->mutable_addition)->CopyFrom(evolve(addition));
```



src/tests/api_tests.cpp
Lines 3821 (patched)


No need for this since the clock is paused.



src/tests/api_tests.cpp
Lines 3834 (patched)


Again, I'm not convinced that this is required.



src/tests/api_tests.cpp
Lines 3836 (patched)


Do we need this?



src/tests/api_tests.cpp
Lines 3841 (patched)


```
offer = offersAfterGrow->at(0);
```



src/tests/api_tests.cpp
Lines 3859-3866 (patched)


```
v1::master::Call::ShrinkVolume* shrinkVolume =
  v1ShrinkVolumeCall.mutable_shrink_volume();
shrinkVolume->...
```



src/tests/api_tests.cpp
Lines 3878 (patched)


No need for this as the clock is paused.



src/tests/api_tests.cpp
Lines 3891 (patched)


Again, not sure if this is needed.



src/tests/api_tests.cpp
Lines 3893 (patched)


Also it seems to me that this is not needed.



src/tests/api_tests.cpp
Lines 3898 (patched)


```
offer = offerAfterShrink->at(0);
```



src/tests/api_tests.cpp
Lines 3903-3945 (patched)


If I'm not mistaken, the test teardown would clean up the work dir, and 
thus the persistent volume, so this is not required. Can you verify that?


- Chun-Hung Hsiao


On April 11, 2018, 9:21 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66227/
> ---
> 
> (Updated April 11, 2018, 9:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-8747
> https://issues.apache.org/jira/browse/MESOS-8747
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test 

Re: Review Request 66559: Made agent flag '--hadoop_home' as optional.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On April 17, 2018, 1:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66559/
> ---
> 
> (Updated April 17, 2018, 1:32 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made agent flag '--hadoop_home' as optional.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md 962211a54177a54b3e38a93aad9af3c7a0f94ecb 
>   docs/operator-http-api.md 10dcac83fa70da5760a5c231665d7498cc168622 
>   src/slave/containerizer/fetcher.cpp 
> 7de57c21c55bc145a78d401bb4224322501c040b 
>   src/slave/flags.hpp beae47f0f8f2178b93a3484d168ce4d71c961841 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd 
> 
> 
> Diff: https://reviews.apache.org/r/66559/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 66679: Made the master send operation status updates when dropping operations.

[Gaston Kleiman]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

This patch makes the master send an operation status update to the
framework with  status `OPERATION_ERROR` when an operation with an
operation ID is dropped.


Diffs
-

  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 


Diff: https://reviews.apache.org/r/66679/diff/1/


Testing
---

`sudo bin/mesos-tests.sh` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66532: Added test for authorization actions for `UPDATE_VOLUME`.

[Greg Mann]

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




src/tests/authorization_tests.cpp
Lines 2155 (patched)


Some of your comments use double-quotes around principal and role names, 
while others use backticks. It looks to me like double-quotes are more common 
in this file - could you make them consistent?


- Greg Mann


On April 10, 2018, 7:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 10, 2018, 7:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `UPDATE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66657: Updated Config initialization for new CLI if file does not exist.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66657]

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

- Mesos Reviewbot


On April 17, 2018, 9:43 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66657/
> ---
> 
> (Updated April 17, 2018, 9:43 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-8779
> https://issues.apache.org/jira/browse/MESOS-8779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Creates an empty configuration file if it does not exist instead
> of throwing an error due to the missing file `config.toml`.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/config.py 
> 6f92622725d8a042a2a728fd38c977ac690ef6be 
> 
> 
> Diff: https://reviews.apache.org/r/66657/diff/1/
> 
> 
> Testing
> ---
> 
> Removed `~/.mesos/config.toml` and ran `mesos config show`.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66532: Added test for authorization actions for `UPDATE_VOLUME`.

[Greg Mann]

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




src/tests/authorization_tests.cpp
Line 1979 (original), 1979 (patched)


Could you also add an end-to-end test of authorization for these operations?


- Greg Mann


On April 10, 2018, 7:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 10, 2018, 7:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `UPDATE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Greg Mann]

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




src/master/master.cpp
Lines 3816-3819 (patched)


Does this comment make sense here? I see that it's included for other 
operations like CREATE above, but it looks to me like this comment only makes 
sense for RESERVE. The fact that users can push just one reservation at a time 
only implies that authorization of the RESERVE operation has been performed for 
all prior reservations.

I would recommend removing this comment, but I think the code here can 
remain the same.

i.e., if a user is attempting to update a volume in role '/foo/bar', we can 
just authorize '/foo/bar' and leave it up to the authorizer if it wants to 
evaluate just '/foo/bar', or to also evaluate role '/foo'.


- Greg Mann


On April 10, 2018, 7:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66531/
> ---
> 
> (Updated April 10, 2018, 7:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new authorization action is modelled after `CreateVolume`, and will
> be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
> corresponding operator APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8ef33210644e7d2924b402a3158b1b38c1fdb464 
>   include/mesos/authorizer/authorizer.proto 
> 1508c01130013d74ed2b2574a2428f38e3d2c064 
>   src/authorizer/local/authorizer.cpp 
> c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
> 
> 
> Diff: https://reviews.apache.org/r/66531/diff/1/
> 
> 
> Testing
> ---
> 
> Manually created ACL and verified that untrusted principals will not allow to 
> grow/shrink volumes.
> Also created unit test in next patch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66052: Added new operator API to grow and shrink persistent volume.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





include/mesos/master/master.proto
Lines 179-186 (patched)


`s/resource_provider_id/provider_id/g`.

Or, we probably don't need to mention anything about `provider_id`, just 
say that `slave_id` must be set if `volume` is an agent-local resource, and 
must be unset if `volume` is an external resource.

Also, a volume from a resource provider could also be a persistent volume, 
so the first sentence is not accurate.



include/mesos/master/master.proto
Lines 197-204 (patched)


Ditto.



include/mesos/v1/master/master.proto
Lines 177-184 (patched)


Ditto.



include/mesos/v1/master/master.proto
Lines 195-202 (patched)


Ditto.


- Chun-Hung Hsiao


On April 17, 2018, 6:36 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66052/
> ---
> 
> (Updated April 17, 2018, 6:36 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-8747
> https://issues.apache.org/jira/browse/MESOS-8747
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The same API could be used in the future to grow or shrink CSI volumes,
> but currently only persistent volumes are supported.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto aa63904a33290a3beda162bbc9f44b56ab04a1e7 
>   include/mesos/v1/master/master.proto 
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
> 
> 
> Diff: https://reviews.apache.org/r/66052/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Greg Mann]

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




src/master/master.hpp
Lines 877 (patched)


s/on a volume/of a volume/



src/master/master.cpp
Lines 4924-4925 (original), 4980-4981 (patched)


You need to finish implementing authorization here, and below for 
SHRINK_VOLUME.


- Greg Mann


On April 10, 2018, 7:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66531/
> ---
> 
> (Updated April 10, 2018, 7:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new authorization action is modelled after `CreateVolume`, and will
> be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
> corresponding operator APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8ef33210644e7d2924b402a3158b1b38c1fdb464 
>   include/mesos/authorizer/authorizer.proto 
> 1508c01130013d74ed2b2574a2428f38e3d2c064 
>   src/authorizer/local/authorizer.cpp 
> c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
> 
> 
> Diff: https://reviews.apache.org/r/66531/diff/1/
> 
> 
> Testing
> ---
> 
> Manually created ACL and verified that untrusted principals will not allow to 
> grow/shrink volumes.
> Also created unit test in next patch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Zhitao Li]

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

(Updated April 17, 2018, 3:53 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Use two variables rather than a pair for reason and message.


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


Repository: mesos


Description
---

These two operations are intended to be non-speculative eventually but
are implemented as speculative right now. To avoid frameworks opt-in to
dangerous behavior, we require that accept can only contain one
`GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.

This is implemented by reuse existing code which already drops `LAUNCH`
or `LAUNCH_GROUP` with proper reason and message.


Diffs (updated)
-

  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 


Diff: https://reviews.apache.org/r/66568/diff/3/

Changes: https://reviews.apache.org/r/66568/diff/2-3/


Testing
---

Behavior tested in https://reviews.apache.org/r/66569/.


Thanks,

Zhitao Li

Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Gaston Kleiman]

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




src/tests/persistent_volume_tests.cpp
Lines 474-475 (patched)


I see that Benjamin recently added this line and the corresponding 
`AWAIT_READY` to many tests.

However I don't think it is necessary here; I'll check with Benjamin to see 
if we can also remove it from other tests in which it is not necessary.



src/tests/persistent_volume_tests.cpp
Lines 502 (patched)


Nit: `Offer offer = offersBeforeCreate->at(0);`

If this test is split into smaller ones, then you can also probable make 
`offer` a const ref.


- Gaston Kleiman


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/master/master.cpp
Lines 4510 (patched)


Since the `createTaskStatus` helper takes an `Option reason` and an 
`Option message`, let's just split this into two variables, e.g., 
`dropReason` and `dropMessage`. similar to what we did elsewhere in the 
codebase, e.g., `default_executor.cpp`.


- Chun-Hung Hsiao


On April 17, 2018, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66568/
> ---
> 
> (Updated April 17, 2018, 5 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two operations are intended to be non-speculative eventually but
> are implemented as speculative right now. To avoid frameworks opt-in to
> dangerous behavior, we require that accept can only contain one
> `GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.
> 
> This is implemented by reuse existing code which already drops `LAUNCH`
> or `LAUNCH_GROUP` with proper reason and message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66568/diff/2/
> 
> 
> Testing
> ---
> 
> Behavior tested in https://reviews.apache.org/r/66569/.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66648: Marked the resource provider API as experimental.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66648 was successfully built and tested.

Reviews applied: `['66616', '66648']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66648

- Mesos Reviewbot Windows


On April 17, 2018, 9:22 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66648/
> ---
> 
> (Updated April 17, 2018, 9:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, Jie 
> Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8787
> https://issues.apache.org/jira/browse/MESOS-8787
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch marks the resource provider API as experimental, and make the
> v0 and v1 protos consistent.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> db7c751bb61fb1ee2421015dcbefc021c3afbdac 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 42bc050ed01a272603a41ab052ed75d799dd76e2 
> 
> 
> Diff: https://reviews.apache.org/r/66648/diff/2/
> 
> 
> Testing
> ---
> 
> No need for testing since only comments are changed.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Zhitao Li]

> On April 17, 2018, 12:04 p.m., Chun-Hung Hsiao wrote:
> > src/master/master.cpp
> > Lines 4525 (patched)
> > 
> >
> > I'm not sure about this. Would `REASON_TASK_INVALID` be more 
> > appropriate?

I honest don't think there is an appropriate reason right now. 
`REASON_TASK_INVALID` would mean payload of task is incorrect, which is also 
not the case here.


- Zhitao


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


On April 17, 2018, 10 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66568/
> ---
> 
> (Updated April 17, 2018, 10 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two operations are intended to be non-speculative eventually but
> are implemented as speculative right now. To avoid frameworks opt-in to
> dangerous behavior, we require that accept can only contain one
> `GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.
> 
> This is implemented by reuse existing code which already drops `LAUNCH`
> or `LAUNCH_GROUP` with proper reason and message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66568/diff/2/
> 
> 
> Testing
> ---
> 
> Behavior tested in https://reviews.apache.org/r/66569/.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Zhitao Li]

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




src/master/master.cpp
Lines 4519-4522 (original), 4543-4546 (patched)


There is an early return in code block, so I think no operation will be 
processed.

However, I think we should call a `drop(...)` on each operations besides 
generating additional TaskStatusUpdates. That will be safe for the offer 
operation feedback in future.

It is not part of this change, so that should be done in a separate patch.

Let me know if


- Zhitao Li


On April 17, 2018, 10 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66568/
> ---
> 
> (Updated April 17, 2018, 10 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two operations are intended to be non-speculative eventually but
> are implemented as speculative right now. To avoid frameworks opt-in to
> dangerous behavior, we require that accept can only contain one
> `GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.
> 
> This is implemented by reuse existing code which already drops `LAUNCH`
> or `LAUNCH_GROUP` with proper reason and message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66568/diff/2/
> 
> 
> Testing
> ---
> 
> Behavior tested in https://reviews.apache.org/r/66569/.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66649 was successfully built and tested.

Reviews applied: `['66649']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66649

- Mesos Reviewbot Windows


On April 17, 2018, 8:22 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66649/
> ---
> 
> (Updated April 17, 2018, 8:22 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current state of support for python in protoc has two serious
> issues:
> 1. The `__init__.py` files necessary to mark a directory as a python
> package aren't generated.
> 2. The import paths in each of the generated .py files do not reflect
> the `--python_path` option passed to `protoc`.
> 
> This results in incomplete code generated, preventing it from being used
> out of the box. To address this issue, we're adding a `pb2gen.sh` script
> to do end-to-end code generation for python protobuf bindings: the
> script generates the bindings based on what's in the `include` dir, then
> postprocesses the generated code to add proper import paths and the
> `__init__.py` files.
> 
> 
> Diffs
> -
> 
>   src/python/lib/pb2gen.sh PRE-CREATION 
>   src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
> 
> 
> Diff: https://reviews.apache.org/r/66649/diff/3/
> 
> 
> Testing
> ---
> 
> 1. under `src/python/lib`, run `pb2gen.sh`
> 2. initialize and activate virtualenv: `virtualenv env && . env/bin/activate`
> 3. install reqs: `pip install -r requirements.in`
> 4. try to import modules from generated python code: `python -c 'from 
> mesos.pb2.mesos.v1.master import master_pb2'`
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Eric Chung]

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

(Updated April 17, 2018, 8:22 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao Li.


Repository: mesos


Description (updated)
---

The current state of support for python in protoc has two serious
issues:
1. The `__init__.py` files necessary to mark a directory as a python
package aren't generated.
2. The import paths in each of the generated .py files do not reflect
the `--python_path` option passed to `protoc`.

This results in incomplete code generated, preventing it from being used
out of the box. To address this issue, we're adding a `pb2gen.sh` script
to do end-to-end code generation for python protobuf bindings: the
script generates the bindings based on what's in the `include` dir, then
postprocesses the generated code to add proper import paths and the
`__init__.py` files.


Diffs (updated)
-

  src/python/lib/pb2gen.sh PRE-CREATION 
  src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 


Diff: https://reviews.apache.org/r/66649/diff/3/

Changes: https://reviews.apache.org/r/66649/diff/2-3/


Testing
---

1. under `src/python/lib`, run `pb2gen.sh`
2. initialize and activate virtualenv: `virtualenv env && . env/bin/activate`
3. install reqs: `pip install -r requirements.in`
4. try to import modules from generated python code: `python -c 'from 
mesos.pb2.mesos.v1.master import master_pb2'`


Thanks,

Eric Chung

Re: Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Eric Chung]

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

(Updated April 17, 2018, 8:13 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao Li.


Repository: mesos


Description (updated)
---

Added pb2gen.sh for generating python protobuf bindings.


Diffs (updated)
-

  src/python/lib/pb2gen.sh PRE-CREATION 
  src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 


Diff: https://reviews.apache.org/r/66649/diff/2/

Changes: https://reviews.apache.org/r/66649/diff/1-2/


Testing
---

1. under `src/python/lib`, run `pb2gen.sh`
2. initialize and activate virtualenv: `virtualenv env && . env/bin/activate`
3. install reqs: `pip install -r requirements.in`
4. try to import modules from generated python code: `python -c 'from 
mesos.pb2.mesos.v1.master import master_pb2'`


Thanks,

Eric Chung

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

> On April 17, 2018, 6:19 p.m., Benno Evers wrote:
> > src/Makefile.am
> > Line 2378 (original), 2369 (patched)
> > 
> >
> > Will this library also get built when using `cmake`?

Please see https://reviews.apache.org/r/66163/. Enablement of SLRP in CMake are 
in the following patches in the chain.


> On April 17, 2018, 6:19 p.m., Benno Evers wrote:
> > src/Makefile.am
> > Lines 2498 (patched)
> > 
> >
> > This seems to be missing in the `CMakeLists.txt`?

Same here. We haven't enabled SLRP in CMake yet before this patch.


- Chun-Hung


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


On April 17, 2018, 3:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 17, 2018, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8749, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66648: Marked the resource provider API as experimental.

[Chun-Hung Hsiao]

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

(Updated April 17, 2018, 7:22 p.m.)


Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, Jie Yu, 
and Jan Schlicht.


Changes
---

Addressed Greg's comment.


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


Repository: mesos


Description (updated)
---

This patch marks the resource provider API as experimental, and make the
v0 and v1 protos consistent.


Diffs (updated)
-

  include/mesos/resource_provider/resource_provider.proto 
db7c751bb61fb1ee2421015dcbefc021c3afbdac 
  include/mesos/v1/resource_provider/resource_provider.proto 
42bc050ed01a272603a41ab052ed75d799dd76e2 


Diff: https://reviews.apache.org/r/66648/diff/2/

Changes: https://reviews.apache.org/r/66648/diff/1-2/


Testing
---

No need for testing since only comments are changed.


Thanks,

Chun-Hung Hsiao

Re: Review Request 66616: Marked volume/block creation and destroy operations as experimental.

[Chun-Hung Hsiao]

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

(Updated April 17, 2018, 7:20 p.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, James DeFelice, and Jie 
Yu.


Changes
---

Unified the way we mark experimental APIs.


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


Repository: mesos


Description (updated)
---

This patch marks the `CREATE_VOLUME`, `DESTROY_VOLUME`, `CREATE_BLOCK`
and `DESTROY_BLOCK` as experimental APIs. It also unifies the way we
mark experimental APIs.


Diffs (updated)
-

  include/mesos/mesos.proto 2c2f63688e579602334add8e242e2da96508cf06 
  include/mesos/v1/mesos.proto edbf5fad47fe56fc46a0133536e46870eb553f32 


Diff: https://reviews.apache.org/r/66616/diff/3/

Changes: https://reviews.apache.org/r/66616/diff/2-3/


Testing
---

No need for testing since only comments are changed.


Thanks,

Chun-Hung Hsiao

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Chun-Hung Hsiao]

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




src/master/master.cpp
Lines 4525 (patched)


I'm not sure about this. Would `REASON_TASK_INVALID` be more appropriate?



src/master/master.cpp
Lines 4519-4522 (original), 4543-4546 (patched)


Shouldn't we drop the other operations as well?

How about moving the validation into `Master::accept()`, e.g., Line 4040?
```
if (accept.operations_size() > 1 && find_if(...)) {
  foreach (foreach (Offer::Operation& operation, 
*accept.mutable_operations()) {
switch (operation.type()) {
  case Offer::Operation::RESERVE:
  ...
  case Offer::Operation::DESTROY_BLOCK: {
send(OPERATION_ERROR, ...);
break;
  }
  case Offer::Operation::LAUNCH:
  case Offer::Operation::LAUNCH_GROUP: {
send(TASK_DROPPED or TASK_LOST, TaskStatus::REASON_TASK_INVALID, 
...);
break;
  }
}
  }
}
```

Or, we could combine the above logic with the conditional branch at Line 
3974.


- Chun-Hung Hsiao


On April 17, 2018, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66568/
> ---
> 
> (Updated April 17, 2018, 5 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two operations are intended to be non-speculative eventually but
> are implemented as speculative right now. To avoid frameworks opt-in to
> dangerous behavior, we require that accept can only contain one
> `GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.
> 
> This is implemented by reuse existing code which already drops `LAUNCH`
> or `LAUNCH_GROUP` with proper reason and message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66568/diff/2/
> 
> 
> Testing
> ---
> 
> Behavior tested in https://reviews.apache.org/r/66569/.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Eric Chung]

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

Review request for mesos.


Repository: mesos


Description
---

Added pb2gen.sh for generating python protobuf bindings.

This change addresses the issue of protobuf bindings for the mesos python 
client. Generating python protobuf bindings is not as straightforward as it 
seems since vanilla `protoc` has two limitiations in terms of python code 
generation:
1. it does not create the `__init__.py` files required for python packages
2. it does not allow arbitrary prefixes to the import paths, even though it 
allows a custo `python_path` when generating code

To this end, this change adds the following:
1. A script (`pb2gen.sh`) for generating protobuf python bindings and 
post-processing the generated python code so that it is usable by the rest of 
the code under `src/python/lib/mesos`.
2. Adds python bindings generated from `pb2gen.sh` using the current set of 
protobuf definitions.


Diffs
-

  src/python/lib/mesos/pb2/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/agent/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/agent/agent_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/allocator/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/allocator/allocator_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/appc/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/appc/spec_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/authentication/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/authentication/authentication_pb2.py 
PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/authorizer/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/authorizer/acls_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/authorizer/authorizer_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/docker/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/docker/spec_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/docker/v1_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/docker/v2_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/executor/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/executor/executor_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/fetcher/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/fetcher/fetcher_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/maintenance/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/maintenance/maintenance_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/master/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/master/master_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/mesos_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/module/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/module/hook_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/module/module_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/oci/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/oci/spec_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/quota/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/quota/quota_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/resource_provider/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/resource_provider/resource_provider_pb2.py 
PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/scheduler/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/scheduler/scheduler_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/slave/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/slave/containerizer_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/slave/oversubscription_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/state/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/state/state_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/uri/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/uri/uri_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/agent/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/agent/agent_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/allocator/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/allocator/allocator_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/executor/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/executor/executor_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/maintenance/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/maintenance/maintenance_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/master/__init__.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/master/master_pb2.py PRE-CREATION 
  src/python/lib/mesos/pb2/mesos/v1/mesos_pb2.py PRE-CREATION 
  

Re: Review Request 66052: Added new operator API to grow and shrink persistent volume.

[Zhitao Li]

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

(Updated April 17, 2018, 11:36 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Review comments.


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


Repository: mesos


Description
---

The same API could be used in the future to grow or shrink CSI volumes,
but currently only persistent volumes are supported.


Diffs (updated)
-

  include/mesos/master/master.proto aa63904a33290a3beda162bbc9f44b56ab04a1e7 
  include/mesos/v1/master/master.proto ddb28f96b2a3a439bb9a829995a9a3015f65ba43 


Diff: https://reviews.apache.org/r/66052/diff/7/

Changes: https://reviews.apache.org/r/66052/diff/6-7/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66051: Implemented operator API to grow and shrink persistent volume.

[Zhitao Li]

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

(Updated April 17, 2018, 11:37 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Removed `resource_provider_id`


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


Repository: mesos


Description
---

These operator APIs is implemented as speculative for now, but
we plan to convert them to non-speculative in the future.


Diffs (updated)
-

  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66051/diff/11/

Changes: https://reviews.apache.org/r/66051/diff/10-11/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Benno Evers]

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


Fix it, then Ship it!





src/Makefile.am
Line 2378 (original), 2369 (patched)


Will this library also get built when using `cmake`?



src/Makefile.am
Lines 2498 (patched)


This seems to be missing in the `CMakeLists.txt`?


- Benno Evers


On April 17, 2018, 3:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 17, 2018, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8749, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On April 12, 2018, 5:27 p.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?
> 
> Chun-Hung Hsiao wrote:
> From the API perspective, `double` is easier to use, but `Value.Scalar` 
> is more consistent with how we represesnt a disk resource.
> 
> From the implementation perspective, I prefer `Value.Scalar` because it's 
> arithmetic operators will do fixed-point conversions for me so I won't forget 
> that.
> 
> Greg Mann wrote:
> My recommendation would be to prioritize easy-of-use in the API over 
> ease-of-implementation when possible :)
> 
> In the implementation, as long as we toss the provided double into a 
> `Scalar::Value` before we do anything with it, I think we could get the same 
> benefit?
> 
> Zhitao Li wrote:
> I feel that `Scalar` is actually more consistent than `double` here. I do 
> not see either API is much easier to use.
> 
> Greg Mann wrote:
> Why is `Scalar` more consistent? I think that `double` would be simpler 
> because it eliminates the nested `value` field, which is not intuitive.
> 
> Is there any reason to make this a `Scalar` other than the fact that it 
> makes our implementation simpler? Do we have other APIs which accept a 
> `Value.Scalar` for similar purposes?
> 
> Chun-Hung Hsiao wrote:
> Actually my point is not about the ease of implementation, but to prevent 
> a careless developer like me ;) to forget to do the fixed-point conversion 
> when doing resource arithmetic. But still, this is a pure implementation 
> concern, not an API concern.

Capturing offline conversation, we will keep `Scalar` but add a comment asking 
reader to see Scalar's comment about precision.


- Zhitao


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


On April 13, 2018, 10:20 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 13, 2018, 10:20 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 455-459 (patched)
> > 
> >
> > Is this enforced somewhere in validation code? Can we check for 
> > expected behavior when a GROW/SHRINK operation is submitted for a MOUNT 
> > volume, rather than simply returning?

I added validation in r/66050 so we drop shrink volume operation on MOUNT. 
There is no logical path for triggering `GROW` on `MOUNT` disk type so I'm not 
writing validation.

For testing, we can either keep the current structure and check that 
`GROW`/`SHRINK` do not work on `MOUNT` (operation will be dropped), or take 
Chun's suggestion to not test them for `MOUNT`. Please let me know.


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 492 (patched)
> > 
> >
> > Is this necessary? Since the clock is paused, is it really possible 
> > that we'll get more offers than expected?

This is very common in our tests (there are more than 500 line matches with 
this comment through `git grep`).

My guess is that `clock::advance` can guarantee at least one offer being 
triggered but there might be some race condition cases which would make the 
test flaky if we do not guard against this?


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 549 (patched)
> > 
> >
> > Is this necessary?

See above comment.


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 553-554 (patched)
> > 
> >
> > Is this necessary? Awaiting on the task status updates may be 
> > sufficient?

See above comment.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66648: Marked the resource provider API as experimental.

[Greg Mann]

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




include/mesos/resource_provider/resource_provider.proto
Lines 27 (patched)


Hmm I feel that "unstable" might imply the existence of known bugs? Perhaps 
something like "NOTE: For the time being this API is subject to change and the 
related feature is experimental."?


- Greg Mann


On April 17, 2018, 3:26 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66648/
> ---
> 
> (Updated April 17, 2018, 3:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8787
> https://issues.apache.org/jira/browse/MESOS-8787
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked the resource provider API as experimental.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> db7c751bb61fb1ee2421015dcbefc021c3afbdac 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 42bc050ed01a272603a41ab052ed75d799dd76e2 
> 
> 
> Diff: https://reviews.apache.org/r/66648/diff/1/
> 
> 
> Testing
> ---
> 
> No need for testing since only comments are changed.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66652: Renamed local_puller to image_tar_puller.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66559, 66650, 66651, 66561, 66562, 66652]

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

- Mesos Reviewbot


On April 16, 2018, 10:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66652/
> ---
> 
> (Updated April 16, 2018, 10:32 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed local_puller to image_tar_puller.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 5ce49ac396b03e8b6d87601ecaa0691d88de21e3 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c664ff807583d587d94b0ab797330d5d3daf7657 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> b703b827a9a00b1c335cd1773c4d3e048eb16d66 
> 
> 
> Diff: https://reviews.apache.org/r/66652/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66671: Updated composing containerizer tests. (WIP)

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66671 was successfully built and tested.

Reviews applied: `['8', '9', '66670', '66671']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66671

- Mesos Reviewbot Windows


On April 17, 2018, 8:23 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66671/
> ---
> 
> (Updated April 17, 2018, 8:23 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8737
> https://issues.apache.org/jira/browse/MESOS-8737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates composing containerizer tests in order to be
> consistent with the unification of `destroy()` and `wait()` return
> types.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 6964ac2f050396a733b04e650e59d6c29c59665d 
> 
> 
> Diff: https://reviews.apache.org/r/66671/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Zhitao Li]

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

(Updated April 17, 2018, 10 a.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Drop all operations if combined with GROW/SHRINK.


Summary (updated)
-

Dropped combined operations with GROW and SHRINK volumes.


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


Repository: mesos


Description (updated)
---

These two operations are intended to be non-speculative eventually but
are implemented as speculative right now. To avoid frameworks opt-in to
dangerous behavior, we require that accept can only contain one
`GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.

This is implemented by reuse existing code which already drops `LAUNCH`
or `LAUNCH_GROUP` with proper reason and message.


Diffs (updated)
-

  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 


Diff: https://reviews.apache.org/r/66568/diff/2/

Changes: https://reviews.apache.org/r/66568/diff/1-2/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Zhitao Li]

> On April 12, 2018, 5:33 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 4986-4994 (patched)
> > 
> >
> > What will happen if these operations are sent to a 1.5 agent which has 
> > the RESOURCE_PROVIDER capability, but does not support the operation?
> 
> Zhitao Li wrote:
> Good catch.
> 
> For a 1.5 agent, I think it will crash at this line: 
> https://github.com/apache/mesos/blob/1.5.0/src/common/protobuf_utils.cpp#L851 
> because it cannot know whether this unknown operation is speculative. I guess 
> that triggers the agent to reregister with master without changing its 
> checkpointed resources.
> 
> In fact, this is much better than attempting to apply, because agent 
> would do a delete followed by recreate and cause data loss on the persistent 
> volume (the fix is in r/66218).
> 
> It seems like agent version was never present in `AgentInfo` so master 
> cannot really know this is an old agent version. Do you want to consider 
> adding that?
> 
> Greg Mann wrote:
> Ah, it looks like the version is included in the 'RegisterSlaveMessage' 
> and 'ReregisterSlaveMessage', and the master places it in the 'Slave' struct: 
> https://github.com/apache/mesos/blob/bd688e4cf9f6f35c9d75aad50b99e1fdeb8104da/src/master/master.cpp#L6586
> 
> Perhaps we should check that the version is >= 1.6.0 rather than checking 
> for the RESOURCE_PROVIDER capability?
> 
> Chun-Hung Hsiao wrote:
> In proto2, since the enum is not defined in 1.5 proto, the enum field 
> will be treated as an unknown field.
> See: https://developers.google.com/protocol-buffers/docs/proto#enum

I'm inclined not to add a version check anymore, but just mention that this is 
not safe to use if operator is using 1.5.x agent with *experimental* 
`RESOURCE_PROVIDER` feature.


- Zhitao


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


On April 17, 2018, 9:58 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 17, 2018, 9:58 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as speculative operations, but
> we will use validation to make them non-speculative on API level so that
> we can transition later without a breaking change.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Zhitao Li]

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

(Updated April 17, 2018, 9:58 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Review comments.


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


Repository: mesos


Description
---

The new offer operations are implemented as speculative operations, but
we will use validation to make them non-speculative on API level so that
we can transition later without a breaking change.


Diffs (updated)
-

  src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
  src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
  src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66050/diff/11/

Changes: https://reviews.apache.org/r/66050/diff/10-11/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66608: Improved support for move-only types in `hashmap`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66608 was successfully built and tested.

Reviews applied: `['66608']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66608

- Mesos Reviewbot Windows


On April 17, 2018, 2:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66608/
> ---
> 
> (Updated April 17, 2018, 2:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While it was already possible to create a `hashmap` over move-only
> values, we still performed a copy in `put`, making it hard to
> dynamically add elements with the expected stout semantics.
> 
> This patch relaxes the requirements on the value argument to `put` so
> that instead of copyable we now only require move-constructible types.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/hashmap.hpp 
> 91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 
> 
> 
> Diff: https://reviews.apache.org/r/66608/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 66670: Ensured that `wait()` and `destroy()` return the same result. (WIP)

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
Zhang.


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


Repository: mesos


Description
---

We need to return the same `ContainerTermination` result for both
`wait()` and `destroy()` for a terminated container. This patch
ensures that for a terminated nested container `destroy()` returns
the same result as for `wait()`.


Diffs
-

  src/slave/containerizer/composing.cpp 
186102c66d373dcd799cadd9fed7d1c8cb894971 
  src/slave/containerizer/mesos/containerizer.cpp 
d1d4c2a1781e2011f7ab8feb79f3c5acd2627007 


Diff: https://reviews.apache.org/r/66670/diff/1/


Testing
---

internal CI


Thanks,

Andrei Budnik

Review Request 66671: Updated composing containerizer tests. (WIP)

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
Zhang.


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


Repository: mesos


Description
---

This patch updates composing containerizer tests in order to be
consistent with the unification of `destroy()` and `wait()` return
types.


Diffs
-

  src/tests/containerizer/composing_containerizer_tests.cpp 
6964ac2f050396a733b04e650e59d6c29c59665d 


Diff: https://reviews.apache.org/r/66671/diff/1/


Testing
---

internal CI


Thanks,

Andrei Budnik

Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer. (WIP)

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
Zhang.


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


Repository: mesos


Description
---

Previously, we were using `destroyed` promise for each container to
guarantee that calling `destroy()` on a container returns a non-empty
value when the destroy-in-progress stops an launch-in-progress using
the next containerizer. Since we are unifying the semantics of the
return type for both `wait()` and `destroy()` operations, we can
remove the `destroyed` promise.


Diffs
-

  src/slave/containerizer/composing.cpp 
186102c66d373dcd799cadd9fed7d1c8cb894971 


Diff: https://reviews.apache.org/r/8/diff/1/


Testing
---

internal CI


Thanks,

Andrei Budnik

Review Request 66669: Added clean up of `containers_` map in composing containerizer. (WIP)

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
Zhang.


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


Repository: mesos


Description
---

This patch adds callbacks on `wait()` and `destroy()` in composing
containerizer to remove a container from the `containers_` map after
the container's termination.


Diffs
-

  src/slave/containerizer/composing.cpp 
186102c66d373dcd799cadd9fed7d1c8cb894971 


Diff: https://reviews.apache.org/r/9/diff/1/


Testing
---

internal CI


Thanks,

Andrei Budnik

Re: Review Request 66648: Marked the resource provider API as experimental.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66398, 66407, 66408, 66409, 66410, 66411, 66418, 66574, 
66575, 66576, 66616, 66648]

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

- Mesos Reviewbot


On April 17, 2018, 3:26 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66648/
> ---
> 
> (Updated April 17, 2018, 3:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8787
> https://issues.apache.org/jira/browse/MESOS-8787
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked the resource provider API as experimental.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> db7c751bb61fb1ee2421015dcbefc021c3afbdac 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 42bc050ed01a272603a41ab052ed75d799dd76e2 
> 
> 
> Diff: https://reviews.apache.org/r/66648/diff/1/
> 
> 
> Testing
> ---
> 
> No need for testing since only comments are changed.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66608: Improved support for move-only types in `hashmap`.

[Benjamin Bannier]

> On April 16, 2018, 6:56 p.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/hashmap.hpp
> > Line 104 (original), 104 (patched)
> > 
> >
> > Reading 
> > https://stackoverflow.com/questions/21035417/is-the-pass-by-value-and-then-move-construct-a-bad-idiom,
> > 
> > I'm not sure what types are usually used as `Value`s in hashmaps. Are 
> > they expensive to move? Are they expensive to copy? Can we say that you 
> > suggestion is a strict improvement? Or at least a reasonable trade-off? I'm 
> > asking because I don't have neither enough experience nor data to make a 
> > decision.
> 
> Benjamin Bannier wrote:
> That's a valid concern. I consciously went for the simpler implementation 
> here, but this being in stout we are in pretty generic territory and it might 
> make sense to go for the slightly less naïve solution.
> 
> What's your feeling @mpark?

@alexr: I updated the patch with an additional overload.

I first tried to consume a generic value type which I then could 
`std::forward`, but unfortunately we put pointers to overloaded functions as 
values into `hashmap` which leads to problems deducing the required type for 
`put`.


- Benjamin


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


On April 17, 2018, 4:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66608/
> ---
> 
> (Updated April 17, 2018, 4:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While it was already possible to create a `hashmap` over move-only
> values, we still performed a copy in `put`, making it hard to
> dynamically add elements with the expected stout semantics.
> 
> This patch relaxes the requirements on the value argument to `put` so
> that instead of copyable we now only require move-constructible types.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/hashmap.hpp 
> 91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 
> 
> 
> Diff: https://reviews.apache.org/r/66608/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66608: Improved support for move-only types in `hashmap`.

[Benjamin Bannier]

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

(Updated April 17, 2018, 4:35 p.m.)


Review request for mesos, Alexander Rukletsov and Michael Park.


Changes
---

Avoided moving when given an lvalue as suggested by alexr.


Repository: mesos


Description (updated)
---

While it was already possible to create a `hashmap` over move-only
values, we still performed a copy in `put`, making it hard to
dynamically add elements with the expected stout semantics.

This patch relaxes the requirements on the value argument to `put` so
that instead of copyable we now only require move-constructible types.


Diffs (updated)
-

  3rdparty/stout/include/stout/hashmap.hpp 
91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 


Diff: https://reviews.apache.org/r/66608/diff/2/

Changes: https://reviews.apache.org/r/66608/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 66666: Added MemoryProfiler to CHANGELOG.

[Benno Evers]

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

Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Added changelog entry for the MemoryProfiler class that was
added in the previous commit.


Diffs
-

  CHANGELOG c02d56d6612c432bb079a15fde84cb30d7455971 


Diff: https://reviews.apache.org/r/6/diff/1/


Testing
---


Thanks,

Benno Evers

Re: Review Request 66635: Fixed potential races when interacting with cgroup subsystem isolators.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66635 was successfully built and tested.

Reviews applied: `['66634', '66635']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635

- Mesos Reviewbot Windows


On April 16, 2018, 3:03 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> ---
> 
> (Updated April 16, 2018, 3:03 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
> 
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9afa02b207e6272836e5a36d69fb48f1f4d02150 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
> ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
> cba21ede59916b0a6e120ecd2b6348b8a946c507 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
> 282a76189579d3ddd61f4aad210ce8e876ff0c25 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 75fab0f1fc7e7403855de0786b8c1155d7599b7f 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> e4977da476a04a708a2e9b9dcc5a2f85da5867f4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> 3be419ad477e9baa329b5388d4c12fa743c7f563 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
> 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> 4c43191251d7411d14ed8b11c79a64ff5d6ccd76 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> d79fda3cf852e50e75dbeeeaeae50b1f471a55a4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> 50348d63f6fffa7662e6b91b5ce4ff730380e708 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
> bcaa16142c4e9882dea88e70095cfb7f223401ef 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
> f778a419e15940d92079d04884887615322791f5 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
> 14c0e79f045fb826d20e476772e017439977dded 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
> d394d793c6bc38e249530c54ddb868ecff7f7865 
> 
> 
> Diff: https://reviews.apache.org/r/66635/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66657: Updated Config initialization for new CLI if file does not exist.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66657 was successfully built and tested.

Reviews applied: `['66657']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66657

- Mesos Reviewbot Windows


On April 17, 2018, 9:43 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66657/
> ---
> 
> (Updated April 17, 2018, 9:43 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-8779
> https://issues.apache.org/jira/browse/MESOS-8779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Creates an empty configuration file if it does not exist instead
> of throwing an error due to the missing file `config.toml`.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/config.py 
> 6f92622725d8a042a2a728fd38c977ac690ef6be 
> 
> 
> Diff: https://reviews.apache.org/r/66657/diff/1/
> 
> 
> Testing
> ---
> 
> Removed `~/.mesos/config.toml` and ran `mesos config show`.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66635: Fixed potential races when interacting with cgroup subsystem isolators.

[Mesos Reviewbot Windows]

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



FAIL: Failed to get dependent review IDs for the current patch.

Failed command: `python.exe 
C:\Jenkins\workspace\mesos-reviewbot-testing\Mesos\utils\get-review-ids.py -r 
66635 -o C:\Users\mesos\AppData\Local\Temp\mesos_dependent_review_ids`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635

- Mesos Reviewbot Windows


On April 16, 2018, 3:03 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> ---
> 
> (Updated April 16, 2018, 3:03 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
> 
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9afa02b207e6272836e5a36d69fb48f1f4d02150 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
> ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
> cba21ede59916b0a6e120ecd2b6348b8a946c507 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
> 282a76189579d3ddd61f4aad210ce8e876ff0c25 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 75fab0f1fc7e7403855de0786b8c1155d7599b7f 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> e4977da476a04a708a2e9b9dcc5a2f85da5867f4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> 3be419ad477e9baa329b5388d4c12fa743c7f563 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
> 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> 4c43191251d7411d14ed8b11c79a64ff5d6ccd76 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> d79fda3cf852e50e75dbeeeaeae50b1f471a55a4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> 50348d63f6fffa7662e6b91b5ce4ff730380e708 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
> bcaa16142c4e9882dea88e70095cfb7f223401ef 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
> f778a419e15940d92079d04884887615322791f5 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
> 14c0e79f045fb826d20e476772e017439977dded 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
> d394d793c6bc38e249530c54ddb868ecff7f7865 
> 
> 
> Diff: https://reviews.apache.org/r/66635/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66643: WIP: Attribute filters.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

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

- Mesos Reviewbot


On April 16, 2018, 2:28 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66643/
> ---
> 
> (Updated April 16, 2018, 2:28 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Attribute filters.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1000968be6a2935a4cac571414d7f06d7df7acf0 
>   src/master/flags.hpp 505786e5631c891a52d9d7db403eff312f461d3d 
>   src/master/flags.cpp dbb35befd612f4be1019293c1889d19296118d07 
> 
> 
> Diff: https://reviews.apache.org/r/66643/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 66657: Updated Config initialization for new CLI if file does not exist.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

Creates an empty configuration file if it does not exist instead
of throwing an error due to the missing file `config.toml`.


Diffs
-

  src/python/cli_new/lib/cli/config.py 6f92622725d8a042a2a728fd38c977ac690ef6be 


Diff: https://reviews.apache.org/r/66657/diff/1/


Testing
---

Removed `~/.mesos/config.toml` and ran `mesos config show`.


Thanks,

Armand Grillet

Re: Review Request 66652: Renamed local_puller to image_tar_puller.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66652 was successfully built and tested.

Reviews applied: `['66559', '66650', '66651', '66561', '66562', '66652']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66652

- Mesos Reviewbot Windows


On April 17, 2018, 5:32 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66652/
> ---
> 
> (Updated April 17, 2018, 5:32 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed local_puller to image_tar_puller.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 5ce49ac396b03e8b6d87601ecaa0691d88de21e3 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c664ff807583d587d94b0ab797330d5d3daf7657 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> b703b827a9a00b1c335cd1773c4d3e048eb16d66 
> 
> 
> Diff: https://reviews.apache.org/r/66652/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>