Review Request 66652: Renamed local_puller to image_tar_puller.

[Gilbert Song]

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

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 66562: Added test for local puller hdfs uri fetcher plugin.

[Gilbert Song]

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

(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
---

Added test for local puller hdfs uri fetcher plugin.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
c664ff807583d587d94b0ab797330d5d3daf7657 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 66561: Supported hdfs fetching in local puller.

[Gilbert Song]

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

(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
---

Supported hdfs fetching in local puller.


Diffs (updated)
-

  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/slave/containerizer/mesos/provisioner/docker/store.cpp 
8b3f07f5027cb90d4b4ed401960494709d3eda5f 


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

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


Testing
---

make check


Thanks,

Gilbert Song

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

[Gilbert Song]

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

(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
---

Made agent flag '--hadoop_home' as optional.


Diffs (updated)
-

  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/

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 66651: Supported host and port in hdfs constructor.

[Gilbert Song]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Supported host and port in hdfs constructor.


Diffs
-

  src/uri/schemes/hdfs.hpp 46b9055cb5404f90445bbf5ee64354242e7b4ca6 


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


Testing
---


Thanks,

Gilbert Song

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

[Gilbert Song]

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

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 66163: Built storage local resource provider with CMake.

[Chun-Hung Hsiao]

> On April 12, 2018, 5:06 p.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 387-390 (patched)
> > 
> >
> > Do you know which targets actually require `ENABLE_GRPC` set as a 
> > pre-processor definition? Since it's new code being added, we should be 
> > able to resovle this TODO before committing.

For now it's just `src/resource_provider/local.cpp`. What's the best way to 
achieve this? IMHO it's not that bad to make this global. ;)


> On April 12, 2018, 5:06 p.m., Andrew Schwartzmeyer wrote:
> > src/resource_provider/storage/CMakeLists.txt
> > Lines 18 (patched)
> > 
> >
> > Nit: I've been aligning the `###`... but I know it's that important.

Oops sorry my bad.


> On April 12, 2018, 5:06 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/CMakeLists.txt
> > Line 237 (original), 250-254 (patched)
> > 
> >
> > So `uri_disk_profile_adaptor` is, what, a module? that depends on (and 
> > links in `mesos`), but then `mesos-tess-interface` (which links in `mesos`) 
> > also links in `uri_disk_profile_adaptor`. So if things are being linked 
> > statically, `mesos-tests` would then then have `libmesos` twice? Sorry, I'm 
> > just trying to sort out the dependency graph in my head. Can you elaborate?

We have the same dependencies for `load_qos_controller`, 
`fixed_resource_estimator` and `logrotate_container_logger` as well. All the 
four modules depends on `libmesos` apperantly, and `mesos-tests-interface` also 
depends on `libmesos`. And since the tests needs these modules, 
`mesos-tests-interface` again depends on all of these modules.

I guess we could remove `mesos` above but it looks weird. Also I think there 
should be no problem to "link" the same `libmesos` twice. This doesn't violade 
the one-definition rule.


- Chun-Hung


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


On April 17, 2018, 3:20 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66163/
> ---
> 
> (Updated April 17, 2018, 3:20 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8698
> https://issues.apache.org/jira/browse/MESOS-8698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds CMake rules for compiling necessary source code for
> building storage local resource provider and related tests.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/examples/CMakeLists.txt e85eed607715d2b3ee596c3c0b51f16682491900 
>   src/resource_provider/storage/CMakeLists.txt PRE-CREATION 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
> 
> 
> Diff: https://reviews.apache.org/r/66163/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66163: Built storage local resource provider with CMake.

[Chun-Hung Hsiao]

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

(Updated April 17, 2018, 3:20 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


Changes
---

Partially addressed Andy's comments.


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


Repository: mesos


Description
---

This patch adds CMake rules for compiling necessary source code for
building storage local resource provider and related tests.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
  src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
  src/examples/CMakeLists.txt e85eed607715d2b3ee596c3c0b51f16682491900 
  src/resource_provider/storage/CMakeLists.txt PRE-CREATION 
  src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 


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

Changes: https://reviews.apache.org/r/66163/diff/3-4/


Testing
---

`sudo make check` with CMake


Thanks,

Chun-Hung Hsiao

Re: Review Request 61118: Building gRPC support in libprocess with CMake.

[Chun-Hung Hsiao]

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

(Updated April 17, 2018, 3:17 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


Changes
---

Partially addressed Andy's comment.


Summary (updated)
-

Building gRPC support in libprocess with CMake.


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


Repository: mesos


Description
---

This patch enables CMake to build code for gRPC support in libprocess
along with tests.


Diffs (updated)
-

  3rdparty/libprocess/src/CMakeLists.txt 
0ce7dac5deea94623530820d173ce3ffe5b42ea4 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
d624402bc9efb43a130a2afdf27cfc1aa69010e4 


Diff: https://reviews.apache.org/r/61118/diff/5/

Changes: https://reviews.apache.org/r/61118/diff/4-5/


Testing
---

`sudo make check` with CMake
`cd 3rdparty/libprocess/src/tests && make libprocess-tests && 
./libprocess-tests`


NOTE: Testing on Windowns is not done yet.


Thanks,

Chun-Hung Hsiao

Re: Review Request 61096: Building gRPC with CMake.

[Chun-Hung Hsiao]

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

(Updated April 17, 2018, 3:16 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


Changes
---

Addressed Andy's comments.


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


Repository: mesos


Description
---

This patch adds an `ENABLE_GRPC` option and builds the bundled gRPC
3rdparty library in CMake.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 488e906486a583e74faceb44d906cee5036a8b99 
  3rdparty/cmake/Versions.cmake 605cbdebe2518c234a55921dc07443546ec045e2 
  cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
  src/cmake/MesosProtobuf.cmake dde034fe4d4251dd7c7397d422136bfa3c9c3ebc 


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

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


Testing
---

`sudo make check` with CMake

NOTE: Testing on Windowns is not done yet.


Thanks,

Chun-Hung Hsiao

Re: Review Request 61096: Building gRPC with CMake.

[Chun-Hung Hsiao]

> On April 12, 2018, 4:41 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/CMakeLists.txt
> > Line 143 (original), 144-145 (patched)
> > 
> >
> > Seems reasonable. Could alternatively setup `ENABLE_GRPC` to set 
> > `ENABLE_SSL` (and therefore also `libevent`?)

As discussed, now I only build `gpr`, `grpc_cpp_plugin`, and `grpc++_unsecure` 
and `grpc_unsecure` or `grpc++` and `grpc` depending on whether `ENABLE_SSL` is 
on.


> On April 12, 2018, 4:41 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/CMakeLists.txt
> > Lines 1012-1015 (patched)
> > 
> >
> > As stated above, the probably won't work on Windows yet. Ths VS 
> > solutions generally stick everything under `Debug` or `Release` folders, 
> > and it's done at build time, not configuration time. It's super annoying.

Temporarily disable the WIN32 build. Can you help me on this since I don't have 
an available Windows machine to test the build? :(


> On April 12, 2018, 4:41 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/CMakeLists.txt
> > Lines 1022-1027 (patched)
> > 
> >
> > Do you want the `C` or `CXX` flags forwarded too?

Thanks for pointing this out. Actually it's not only the flags, but the 
compiler themselves should be forwarded as well.


- Chun-Hung


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


On April 12, 2018, 3:22 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61096/
> ---
> 
> (Updated April 12, 2018, 3:22 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7881
> https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an `ENABLE_GRPC` option and builds the bundled gRPC
> 3rdparty library in CMake.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 488e906486a583e74faceb44d906cee5036a8b99 
>   3rdparty/cmake/Versions.cmake 93f0322c1ac926bcfdcd4c1cfd9ba9f22bcf7099 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
>   src/cmake/MesosProtobuf.cmake dde034fe4d4251dd7c7397d422136bfa3c9c3ebc 
> 
> 
> Diff: https://reviews.apache.org/r/61096/diff/6/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Chun-Hung Hsiao]

---
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.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

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 (updated)
-

  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/

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 66628: Added role tracking unit tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66628 was successfully built and tested.

Reviews applied: `['66624', '66625', '65939', '65940', '66628']`

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

- Mesos Reviewbot Windows


On April 17, 2018, 5:12 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66628/
> ---
> 
> (Updated April 17, 2018, 5:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, 
> Michael Park, and Meng Zhu.
> 
> 
> Bugs: MESOS-8069
> https://issues.apache.org/jira/browse/MESOS-8069
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added role tracking unit tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/tests/role_tracking_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66628/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

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

[James DeFelice]

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


Ship it!




Ship It!

- James DeFelice


On April 16, 2018, 11:31 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66616/
> ---
> 
> (Updated April 16, 2018, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-8787
> https://issues.apache.org/jira/browse/MESOS-8787
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked volume/block creation and destroy operations as experimental.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66616/diff/2/
> 
> 
> Testing
> ---
> 
> No need for testing since only comments are changed.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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/#review201280
---



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 16, 2018, 11:32 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66648/
> ---
> 
> (Updated April 16, 2018, 11:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, 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 66050: Implemented grow and shrink of persistent volumes.

[Zhitao Li]

> On April 16, 2018, 10:38 a.m., Gaston Kleiman wrote:
> > src/master/validation.cpp
> > Lines 2348-2351 (patched)
> > 
> >
> > Isn't it easier to do the following?
> > 
> > ```
> > if (growVoluem.addition.scalar.value() <= 0) {
> > ...
> > ```
> 
> Chun-Hung Hsiao wrote:
> The `Scalar` comparison contains a fixed-point conversion: 
> https://github.com/apache/mesos/blob/master/src/common/values.cpp#L103
> 
> Gaston Kleiman wrote:
> This isn't obvious to the uninformed reader (aka me). Can we add a 
> comment here?
> 
> Chun-Hung Hsiao wrote:
> To be honest, the fixed-point conversion seems not necessary here. But 
> this is consistent with how we check for empty resource in 
> `src/common/resources.cpp`.

I think they are: we don't want a `GROW_VOLUME.addition = 0.1` since that 
could round off elsewhere and trigger strange problems. I will add a comment 
here.


- Zhitao


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


On April 16, 2018, 10:09 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 10:09 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66641: Added `FsTest.Open` to cover `os::open()`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66420, 66421, 66422, 66423, 66424, 66425, 66426, 66427, 
66428, 66455, 66429, 66430, 66431, 66432, 66434, 66435, 66436, 66437, 66433, 
66438, 66439, 66440, 66442, 66443, 66444, 66445, 66578, 66641]

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, 1:24 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66641/
> ---
> 
> (Updated April 16, 2018, 1:24 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, John Kordich, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `FsTest.Open` to cover `os::open()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> c190baa2230298e428d4034b90dccffb59b4e710 
> 
> 
> Diff: https://reviews.apache.org/r/66641/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 61118: Libprocess: Building gRPC support with CMake.

[Chun-Hung Hsiao]

> On April 12, 2018, 4:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/tests/CMakeLists.txt
> > Lines 91 (patched)
> > 
> >
> > I think this should instead be appended above. That is, after setting 
> > `GRPC_TESTS_PROTOS`, do a `list(APPEND PROCESS_TESTS_SRC 
> > ${GRPC_TESTS_PROTOS})`, to be consistent with the rest.

Since the files in `${GRPC_TESTS_PROTOS}` are generated files, should we put 
them into `${PROCESS_TESTS_SRC}`?


- Chun-Hung


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


On March 19, 2018, 11:53 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61118/
> ---
> 
> (Updated March 19, 2018, 11:53 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7881
> https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables CMake to build code for gRPC support in libprocess
> along with tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> d624402bc9efb43a130a2afdf27cfc1aa69010e4 
> 
> 
> Diff: https://reviews.apache.org/r/61118/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> `cd 3rdparty/libprocess/src/tests && make libprocess-tests && 
> ./libprocess-tests`
> 
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 66628: Added role tracking unit tests.

[Till Toenshoff]

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, 
Michael Park, and Meng Zhu.


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


Repository: mesos


Description
---

Added role tracking unit tests.


Diffs
-

  src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
  src/tests/role_tracking_tests.cpp PRE-CREATION 


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


Testing
---

make check


Thanks,

Till Toenshoff

Review Request 66625: Updated role-endpoint tests for Role::resources rename.

[Till Toenshoff]

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

Review request for mesos, Benjamin Mahler, Kapil Arya, Meng Zhu, and Vinod Kone.


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


Repository: mesos


Description
---

Updated role-endpoint tests for Role::resources rename.


Diffs
-

  src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
  src/tests/role_tests.cpp a609ed27ef828ca82efc0d269669cda92e5f50a1 


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


Testing
---

make check


Thanks,

Till Toenshoff

Review Request 66624: Deprecated Role::resources in favor of Role::allocated.

[Till Toenshoff]

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

Review request for mesos, Benjamin Mahler, Kapil Arya, Meng Zhu, and Vinod Kone.


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


Repository: mesos


Description
---

Deprecates the `resources` member of the `Role` Message in mesos.proto
(V0 + V1), replacing it by `allocated`. Follows that deprecation on
role-related endpoints, adding allocated to both "/roles" as well as
"GET_ROLES".


Diffs
-

  CHANGELOG c02d56d6612c432bb079a15fde84cb30d7455971 
  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 


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


Testing
---

make check


Thanks,

Till Toenshoff

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/
---

Review request for mesos, Benjamin Bannier, 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 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 16, 2018, 11:31 p.m.)


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


Changes
---

Addressed James' comment.


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


Repository: mesos


Description
---

Marked volume/block creation and destroy operations as experimental.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 


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

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


Testing
---

No need for testing since only comments are changed.


Thanks,

Chun-Hung Hsiao

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/#review201273
---




include/mesos/master/master.proto
Lines 82-83 (patched)


Should we make the comments aligned with the above ones?



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


After rethink about this, the resource itself has a `provide_id` field, so 
it seems to me that the `resource_provider_id` is not needed. How about 
removing `resource_provider_id` and explaining in the comments that `slave_id` 
is set if `volume` is a local resource, and is unset if it is an external 
resource?

We can always put this field back once when we really need it.



include/mesos/master/master.proto
Lines 198-207 (patched)


Ditto.



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


Ditto.



include/mesos/v1/master/master.proto
Lines 196-205 (patched)


Ditto.


- Chun-Hung Hsiao


On March 28, 2018, 6:28 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66052/
> ---
> 
> (Updated March 28, 2018, 6:28 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/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Chun-Hung Hsiao]

> On April 13, 2018, 12:27 a.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?

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.


- Chun-Hung


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


On April 13, 2018, 5:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 13, 2018, 5:20 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 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 66621: Add support for alg RS256 to JWT library.

[Clément Michaud]

> On avr. 16, 2018, 9:52 après-midi, Mesos Reviewbot Windows wrote:
> > FAIL: Failed to apply the current review.
> > 
> > Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621
> > 
> > Relevant logs:
> > 
> > - 
> > [apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):
> > 
> > ```
> > Traceback (most recent call last):
> >   File ".\support\apply-reviews.py", line 447, in 
> > main()
> >   File ".\support\apply-reviews.py", line 442, in main
> > reviewboard(options)
> >   File ".\support\apply-reviews.py", line 432, in reviewboard
> > apply_review(options)
> >   File ".\support\apply-reviews.py", line 163, in apply_review
> > commit_patch(options)
> >   File ".\support\apply-reviews.py", line 292, in commit_patch
> > shell(cmd, options['dry_run'])
> >   File ".\support\apply-reviews.py", line 147, in shell
> > error_code = subprocess.call(command, stderr=subprocess.STDOUT, 
> > shell=True)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
> > return Popen(*popenargs, **kwargs).wait()
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
> > errread, errwrite)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 610, in 
> > _execute_child
> > args = '{} /c "{}"'.format (comspec, args)
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in 
> > position 23: ordinal not in range(128)
> > ```
> 
> Clément Michaud wrote:
> Can anyone help me find what is wrong with the bot?The script is running 
> well on my laptop.

Can it be the accent in my first name?


- Clément


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


On avr. 16, 2018, 9 après-midi, Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated avr. 16, 2018, 9 après-midi)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-8788
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for alg RS256 to JWT library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> I've added the same tests than the ones for HS256 (i.e., validation in 
> following cases: bad header, bad payload, unknown alg, unsupported alg, valid 
> token etc.. and creation of a valid token).
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

[Clément Michaud]

> On avr. 16, 2018, 9:52 après-midi, Mesos Reviewbot Windows wrote:
> > FAIL: Failed to apply the current review.
> > 
> > Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621
> > 
> > Relevant logs:
> > 
> > - 
> > [apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):
> > 
> > ```
> > Traceback (most recent call last):
> >   File ".\support\apply-reviews.py", line 447, in 
> > main()
> >   File ".\support\apply-reviews.py", line 442, in main
> > reviewboard(options)
> >   File ".\support\apply-reviews.py", line 432, in reviewboard
> > apply_review(options)
> >   File ".\support\apply-reviews.py", line 163, in apply_review
> > commit_patch(options)
> >   File ".\support\apply-reviews.py", line 292, in commit_patch
> > shell(cmd, options['dry_run'])
> >   File ".\support\apply-reviews.py", line 147, in shell
> > error_code = subprocess.call(command, stderr=subprocess.STDOUT, 
> > shell=True)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
> > return Popen(*popenargs, **kwargs).wait()
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
> > errread, errwrite)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 610, in 
> > _execute_child
> > args = '{} /c "{}"'.format (comspec, args)
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in 
> > position 23: ordinal not in range(128)
> > ```

Can anyone help me find what is wrong with the bot?The script is running well 
on my laptop.


- Clément


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


On avr. 16, 2018, 9 après-midi, Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated avr. 16, 2018, 9 après-midi)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-8788
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for alg RS256 to JWT library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/2/
> 
> 
> Testing
> ---
> 
> I've added the same tests than the ones for HS256 (i.e., validation in 
> following cases: bad header, bad payload, unknown alg, unsupported alg, valid 
> token etc.. and creation of a valid token).
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

Re: Review Request 66643: WIP: Attribute filters.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66643']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

- 
[mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66643/logs/mesos-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170):
 warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543):
 warning C4996: 'strcpy': This function or variable may be unsafe. Consider 
using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 

Re: Review Request 66569: Added a test to verify that grow and shrink cannot be combined.

[Chun-Hung Hsiao]

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




src/tests/persistent_volume_tests.cpp
Lines 691-695 (patched)


Since this test just verifies that the grow and shrink operations cannot be 
combined with other operations, I was wondering if there's a way to check this 
without worry about the different behaviors produced by PATH or MOUNT.

Alternatively, if you take my suggestion to create another 
`PersistentVolumeResizeTest` fixture in the previous patch, we can move this 
test into that fixture.



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


No need for this.



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


No need for this if the clock is already paused.



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


Let's move this to the beginning of this test to make it clear that this 
test pauses the clock.



src/tests/persistent_volume_tests.cpp
Lines 775-781 (patched)


As discussed, I'm not sure if this is the semantics we want to enforce. If 
we end up rejecting the whole `ACCEPT` call, then we could simplify the test by:

1. Starting with a persistent volume, some free disk and some CPU.
2. Open receiving an offer, accept it with reserving the CPU and growing 
the persistent volume. Different type of resources are used here to make sure 
it's not just for preventing pipelining.
3. Verify that the `ACCEPT` call fails by checking that the next offer 
contains the original resources.



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


No need for this.



src/tests/persistent_volume_tests.cpp
Lines 814-861 (patched)


How about moving the `SHRINK_VOLUME` test into another test? Let's keep the 
test focused.


- Chun-Hung Hsiao


On April 11, 2018, 9:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66569/
> ---
> 
> (Updated April 11, 2018, 9:19 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
> ---
> 
> Added a test to verify that grow and shrink cannot be combined.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66569/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66344: Supported non-speculative operations on agent default resources.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [66344, 66049, 66218]

Failed command: python support/apply-reviews.py -n -r 66344

Error:
2018-04-16 22:00:48 URL:https://reviews.apache.org/r/66344/diff/raw/ 
[3810/3810] -> "66344.patch" [1]
error: patch failed: src/slave/slave.cpp:4298
error: src/slave/slave.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/22199/console

- Mesos Reviewbot


On March 28, 2018, 9:41 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66344/
> ---
> 
> (Updated March 28, 2018, 9:41 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
> ---
> 
> We use `getResourceConversions` to calculate `conversions`,
> apply that to agent default resources if not on resource provider,
> and send `UpdateOperationStatusMessage` message back to master.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
> 
> 
> Diff: https://reviews.apache.org/r/66344/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`

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

Relevant logs:

- 
[apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):

```
Traceback (most recent call last):
  File ".\support\apply-reviews.py", line 447, in 
main()
  File ".\support\apply-reviews.py", line 442, in main
reviewboard(options)
  File ".\support\apply-reviews.py", line 432, in reviewboard
apply_review(options)
  File ".\support\apply-reviews.py", line 163, in apply_review
commit_patch(options)
  File ".\support\apply-reviews.py", line 292, in commit_patch
shell(cmd, options['dry_run'])
  File ".\support\apply-reviews.py", line 147, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
  File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
  File "C:\Program Files\Python27\lib\subprocess.py", line 610, in 
_execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 
23: ordinal not in range(128)
```

- Mesos Reviewbot Windows


On April 16, 2018, 9 p.m., Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated April 16, 2018, 9 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-8788
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for alg RS256 to JWT library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/2/
> 
> 
> Testing
> ---
> 
> I've added the same tests than the ones for HS256 (i.e., validation in 
> following cases: bad header, bad payload, unknown alg, unsupported alg, valid 
> token etc.. and creation of a valid token).
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

Re: Review Request 66641: Added `FsTest.Open` to cover `os::open()`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66641 was successfully built and tested.

Reviews applied: `['66423', '66424', '66425', '66426', '66427', '66428', 
'66455', '66429', '66430', '66431', '66432', '66434', '66435', '66436', 
'66437', '66433', '66438', '66439', '66440', '66442', '66443', '66444', 
'66445', '66578', '66641']`

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

- Mesos Reviewbot Windows


On April 16, 2018, 1:24 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66641/
> ---
> 
> (Updated April 16, 2018, 1:24 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, John Kordich, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `FsTest.Open` to cover `os::open()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> c190baa2230298e428d4034b90dccffb59b4e710 
> 
> 
> Diff: https://reviews.apache.org/r/66641/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 66643: WIP: Attribute filters.

[Benno Evers]

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

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

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

[Greg Mann]

> On April 13, 2018, 12:27 a.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.

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?


- Greg


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


On April 13, 2018, 5:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 13, 2018, 5:20 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 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.

[Greg Mann]

> On April 16, 2018, 5:57 p.m., Zhitao Li wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 541-542 (patched)
> > 
> >
> > Let's capture this message and test its content to make sure proper 
> > operation is forwarded.

I think it's OK to rely on the successful task launch to infer that the volume 
was created successfully. I would recommend removing this future.


> On April 16, 2018, 5:57 p.m., Zhitao Li wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 597-603 (patched)
> > 
> >
> > This was necessary if the operation is implemented as non-speculative, 
> > because master/allocator only sends out `converted` resources after 
> > receiving the feedback from agent. If we don't ensure this is processed, 
> > the offer could be subject to race condition (I think).
> > 
> > Now that we changed the course, this may not be necessary in 1.6, but 
> > we'd need them again once we get to MESOS-8791.
> > 
> > Please let me know if you think keeping it is still a bad idea.

I think if the framework accepts the offer and then you immediately call 
`Clock::settle()`, you can still be sure that the next offer you get will 
contain the volume. I would recommend getting rid of the `growVolumeMessage` 
and `shrinkVolumeMessage` futures.


- Greg


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


On April 11, 2018, 9: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, 9: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 66621: Add support for alg RS256 to JWT library.

[Clément Michaud]

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

(Updated avr. 16, 2018, 9 après-midi)


Review request for mesos.


Changes
---

Fix style issues and add one test validating RS256 token with wrong public key


Summary (updated)
-

Add support for alg RS256 to JWT library.


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

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788


Repository: mesos


Description (updated)
---

Add support for alg RS256 to JWT library.


Diffs (updated)
-

  3rdparty/libprocess/include/process/jwt.hpp 
768cbf6fa91537ff9f45f236f4033097c5cea959 
  3rdparty/libprocess/include/process/ssl/utilities.hpp 
b7cc31c33fd35c93754407f8b350eeb993177f1d 
  3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
  3rdparty/libprocess/src/ssl/utilities.cpp 
4d3727daf53ec62a19255da5a9804d342e770ec2 
  3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/jwt_tests.cpp 
eb36a9aed3b11208c7cdc6f20b5347f46821a207 


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

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


Testing
---

I've added the same tests than the ones for HS256 (i.e., validation in 
following cases: bad header, bad payload, unknown alg, unsupported alg, valid 
token etc.. and creation of a valid token).


Thanks,

Clément Michaud

Re: Review Request 66615: Adopted `createTask` helper API in Docker containerizer tests.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On April 16, 2018, 9:51 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66615/
> ---
> 
> (Updated April 16, 2018, 9:51 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and 
> Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Docker containerizer tests were manually creating the `TaskInfo`
> for the tasks they wanted to launch. We can remove some of that
> boilerplate by adopting the `createTask` helper.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 847258daadf3c37d9071151616b18fc79d850ce8 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
> 
> 
> Diff: https://reviews.apache.org/r/66615/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 27).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53267: Added a gauge to track active subscribers.

[James Peach]

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



Please document the new metric in `monitoring.md`.


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


Nit. This should be in `master.cpp` for consistency with the other gauges.


- James Peach


On April 12, 2018, 9:33 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53267/
> ---
> 
> (Updated April 12, 2018, 9:33 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jason Lai, and James Peach.
> 
> 
> Bugs: MESOS-6499
> https://issues.apache.org/jira/browse/MESOS-6499
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a gauge to track active subscribers.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
>   src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
> 
> 
> Diff: https://reviews.apache.org/r/53267/diff/2/
> 
> 
> Testing
> ---
> 
> Ran this on master with a client keeps subscribing and disconnecting. 
> Observes that gauge gets updated and new log line is printed.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 66641: Added `FsTest.Open` to cover `os::open()`.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, John Kordich, and Joseph Wu.


Repository: mesos


Description
---

Added `FsTest.Open` to cover `os::open()`.


Diffs
-

  3rdparty/stout/tests/os/filesystem_tests.cpp 
c190baa2230298e428d4034b90dccffb59b4e710 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

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

[Chun-Hung Hsiao]

> On April 14, 2018, 1:52 a.m., Chun-Hung Hsiao wrote:
> > src/master/validation.cpp
> > Lines 2336 (patched)
> > 
> >
> > For consistency, I prefer just `"Invalid resource: "`.
> > 
> > Or if you want to make it explicit, maybe:
> > `"Invalid resource in the 'volume' field: "`, or
> > `Invalid resource in the 'GrowVolume.volume' field: "`.
> > 
> > Ditto below for 'addition':
> > `"Invalid resource: "`, or
> > `"Invalid additional resource: "`, or
> > `"Invalid resource in the 'addition' field: "`, or
> > `"Invalid resource in the 'GrowVolume.addition' field: "`.
> > 
> > Note that I use `GrowVolume` instead of `grow_volume` since there is no 
> > guarantee that the field being validated by this funciton is called 
> > `grow_volume`.

Reopened this issue because of `grow_volume` in the error message. Please leave 
your thoughts if you have objections. Thanks!


> On April 14, 2018, 1:52 a.m., Chun-Hung Hsiao wrote:
> > src/master/validation.cpp
> > Lines 2387 (patched)
> > 
> >
> > Similar to the reason I mentioned above, let's do either of the 
> > followings:
> > 
> > `"Invalid resource: "`
> > `"Invalid resource in the 'volume' field: "`
> > `Invalid resource in the 'ShrinkVolume.volume' field: "`

Reopened this issue because of `shrink_volume` in the error message.


- Chun-Hung


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


On April 16, 2018, 5:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 5:09 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
> ---
> 
> 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Chun-Hung Hsiao]

> On April 16, 2018, 5:38 p.m., Gaston Kleiman wrote:
> > src/master/validation.cpp
> > Lines 2348-2351 (patched)
> > 
> >
> > Isn't it easier to do the following?
> > 
> > ```
> > if (growVoluem.addition.scalar.value() <= 0) {
> > ...
> > ```
> 
> Chun-Hung Hsiao wrote:
> The `Scalar` comparison contains a fixed-point conversion: 
> https://github.com/apache/mesos/blob/master/src/common/values.cpp#L103
> 
> Gaston Kleiman wrote:
> This isn't obvious to the uninformed reader (aka me). Can we add a 
> comment here?

To be honest, the fixed-point conversion seems not necessary here. But this is 
consistent with how we check for empty resource in `src/common/resources.cpp`.


- Chun-Hung


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


On April 16, 2018, 5:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 5:09 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
> ---
> 
> 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66615, 66637]

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, 5:31 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66637/
> ---
> 
> (Updated April 16, 2018, 5:31 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and 
> Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Docker containerizer tests all create a Docker `ContainerInfo`.
> We can reduce the amount of copy/paste boilerplate by adding a
> simple helper function to create a `ContainerInfo` with a Docker
> image.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 847258daadf3c37d9071151616b18fc79d850ce8 
> 
> 
> Diff: https://reviews.apache.org/r/66637/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Gaston Kleiman]

> On April 16, 2018, 10:38 a.m., Gaston Kleiman wrote:
> > src/master/validation.cpp
> > Lines 2348-2351 (patched)
> > 
> >
> > Isn't it easier to do the following?
> > 
> > ```
> > if (growVoluem.addition.scalar.value() <= 0) {
> > ...
> > ```
> 
> Chun-Hung Hsiao wrote:
> The `Scalar` comparison contains a fixed-point conversion: 
> https://github.com/apache/mesos/blob/master/src/common/values.cpp#L103

This isn't obvious to the uninformed reader (aka me). Can we add a comment here?


- Gaston


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


On April 16, 2018, 10:09 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 10:09 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66344: Supported non-speculative operations on agent default resources.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66344`

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

Relevant logs:

- 
[apply-review-66344-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66344/logs/apply-review-66344-stdout.log):

```
error: patch failed: src/slave/slave.cpp:4298
error: src/slave/slave.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On March 28, 2018, 9:41 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66344/
> ---
> 
> (Updated March 28, 2018, 9:41 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
> ---
> 
> We use `getResourceConversions` to calculate `conversions`,
> apply that to agent default resources if not on resource provider,
> and send `UpdateOperationStatusMessage` message back to master.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
> 
> 
> Diff: https://reviews.apache.org/r/66344/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66615', '66637']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

- 
[mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66637/logs/mesos-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543):
 warning C4996: 'strcpy': This function or variable may be unsafe. Consider 
using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569):
 warning 

Re: Review Request 66344: Supported non-speculative operations on agent default resources.

[Gaston Kleiman]

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




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


The agent should retry this update for as long as possible, so it should 
set a status UUID.


- Gaston Kleiman


On March 28, 2018, 2:41 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66344/
> ---
> 
> (Updated March 28, 2018, 2:41 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
> ---
> 
> We use `getResourceConversions` to calculate `conversions`,
> apply that to agent default resources if not on resource provider,
> and send `UpdateOperationStatusMessage` message back to master.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
> 
> 
> Diff: https://reviews.apache.org/r/66344/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Zhitao Li]

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



Please consider unreplied comments as "will do".


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


Is the goal of splitting the test to have more focused tests? Otherwise, 
just taking out the task launching part from the code seems resulting in 
minimum amount of code.

The file check actually verifies the fix for r/66218, which I think is 
something pretty important (not losing data during a grow/shrink). Only 
checking path exists for the volume would not capture that behavior.



src/tests/persistent_volume_tests.cpp
Lines 455-459 (patched)


I think we discussed about not allowing this but I forgot add this to 
validation. I'll update parent patch to include that validation.

If we will drop any `GROW`/`SHRINK` operation, then we can verify that 
`GROW` does not trigger any `ApplyOperationMessage` and framework will be 
offered original volume as is and keep this well tested.



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


I do not see a possibility. Let me try to drop it and see what happens.



src/tests/persistent_volume_tests.cpp
Lines 541-542 (patched)


Let's capture this message and test its content to make sure proper 
operation is forwarded.



src/tests/persistent_volume_tests.cpp
Lines 597-603 (patched)


This was necessary if the operation is implemented as non-speculative, 
because master/allocator only sends out `converted` resources after receiving 
the feedback from agent. If we don't ensure this is processed, the offer could 
be subject to race condition (I think).

Now that we changed the course, this may not be necessary in 1.6, but we'd 
need them again once we get to MESOS-8791.

Please let me know if you think keeping it is still a bad idea.



src/tests/persistent_volume_tests.cpp
Lines 677-678 (patched)


Resources::contains only matches a volume if their size perfectly matches, 
so I think it is sufficient.


- Zhitao Li


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 66050: Implemented grow and shrink of persistent volumes.

[Chun-Hung Hsiao]

> On April 16, 2018, 5:38 p.m., Gaston Kleiman wrote:
> > src/master/validation.cpp
> > Lines 2348-2351 (patched)
> > 
> >
> > Isn't it easier to do the following?
> > 
> > ```
> > if (growVoluem.addition.scalar.value() <= 0) {
> > ...
> > ```

The `Scalar` comparison contains a fixed-point conversion: 
https://github.com/apache/mesos/blob/master/src/common/values.cpp#L103


- Chun-Hung


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


On April 16, 2018, 5:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 5:09 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
> ---
> 
> 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66634, 66635]

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, 8:03 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> ---
> 
> (Updated April 16, 2018, 8:03 a.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
> style 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/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Chun-Hung Hsiao]

> On April 13, 2018, 12:33 a.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?

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


- Chun-Hung


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


On April 16, 2018, 5:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 5:09 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
> ---
> 
> 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Greg Mann]

> On April 13, 2018, 12:33 a.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?

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?


- Greg


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


On April 16, 2018, 5:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 5:09 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
> ---
> 
> 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.

[Gaston Kleiman]

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


Ship it!




Thanks again for the cleanup!

Just a random comment, we could start doing:

```
*task.mutable_container() = createDockerInfo("alpine");
```

Instead of:

```
task.mutable_container()->CopyFrom(createDockerInfo("alpine"));
```

But that would not be consistent with what we do in most places in our tests, 
so I didn't open an issue =).

- Gaston Kleiman


On April 16, 2018, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66637/
> ---
> 
> (Updated April 16, 2018, 10:31 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and 
> Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Docker containerizer tests all create a Docker `ContainerInfo`.
> We can reduce the amount of copy/paste boilerplate by adding a
> simple helper function to create a `ContainerInfo` with a Docker
> image.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 847258daadf3c37d9071151616b18fc79d850ce8 
> 
> 
> Diff: https://reviews.apache.org/r/66637/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Gaston Kleiman]

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




src/master/master.cpp
Lines 4931-4932 (patched)


I'd change this to:

```
"Agent " + stringify(slave) + " does not have the required 
RESOURCE_PROVIDER capability"
```

To be consistent with what `Master::accept()` returns when an operation ID 
is specified, but the agent doesn't support `RESOURCE_PROVIDER`.



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


Shouldn't it be `on agent`?



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


I don't think we need to add this line in this patch.

I did notice that we are not consistent in always adding an empty line 
after the `drop(...)`. We might want to do a sweep on this function.



src/master/validation.cpp
Lines 2337 (patched)


We don't normally include the type of operation in the error.

If we do decide to include it for these new operations, I'd write it in 
upper case.



src/master/validation.cpp
Lines 2348-2351 (patched)


Isn't it easier to do the following?

```
if (growVoluem.addition.scalar.value() <= 0) {
...
```


- Gaston Kleiman


On April 16, 2018, 10:09 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 10:09 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/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.

[James Peach]

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

(Updated April 16, 2018, 5:31 p.m.)


Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and Zhitao 
Li.


Repository: mesos


Description
---

The Docker containerizer tests all create a Docker `ContainerInfo`.
We can reduce the amount of copy/paste boilerplate by adding a
simple helper function to create a `ContainerInfo` with a Docker
image.


Diffs (updated)
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
847258daadf3c37d9071151616b18fc79d850ce8 


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

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


Testing
---

sudo make check (Fedora 27)


Thanks,

James Peach

Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66615', '66637']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

- 
[mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66637/logs/mesos-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543):
 warning C4996: 'strcpy': This function or variable may be unsafe. Consider 
using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569):
 warning 

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 16, 2018, 10:09 a.m.)


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


Changes
---

Style 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/10/

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


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:
> >

Will do for any comment not explicitly replied.


> 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?

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?


- Zhitao


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


On April 16, 2018, 9:42 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated April 16, 2018, 9:42 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/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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.

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?


- Benjamin


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


On April 13, 2018, 3:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66608/
> ---
> 
> (Updated April 13, 2018, 3:09 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 releases the requirements on the value argument to `put` so
> that instead of copyable we now only require move-constructible.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/hashmap.hpp 
> 91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 
> 
> 
> Diff: https://reviews.apache.org/r/66608/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.

[Gaston Kleiman]

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




src/tests/containerizer/docker_containerizer_tests.cpp
Lines 107-111 (patched)


Nit:

I'd do:

```
ContainerInfo::DockerInfo* dockerInfo = containerInfo.mutable_docker();
dockerInfo->set_image(imageName);

return containerInfo;
```



src/tests/containerizer/docker_containerizer_tests.cpp
Line 378 (original)


The default networking is `HOST`, so this line shouldn't be removed.


- Gaston Kleiman


On April 16, 2018, 9:52 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66637/
> ---
> 
> (Updated April 16, 2018, 9:52 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and 
> Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Docker containerizer tests all create a Docker `ContainerInfo`.
> We can reduce the amount of copy/paste boilerplate by adding a
> simple helper function to create a `ContainerInfo` with a Docker
> image.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 847258daadf3c37d9071151616b18fc79d850ce8 
> 
> 
> Diff: https://reviews.apache.org/r/66637/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Alexander Rukletsov]

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




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.


- Alexander Rukletsov


On April 13, 2018, 1:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66608/
> ---
> 
> (Updated April 13, 2018, 1:09 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 releases the requirements on the value argument to `put` so
> that instead of copyable we now only require move-constructible.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/hashmap.hpp 
> 91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 
> 
> 
> Diff: https://reviews.apache.org/r/66608/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.

[James Peach]

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

Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and Zhitao 
Li.


Repository: mesos


Description
---

The Docker containerizer tests all create a Docker `ContainerInfo`.
We can reduce the amount of copy/paste boilerplate by adding a
simple helper function to create a `ContainerInfo` with a Docker
image.


Diffs
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
847258daadf3c37d9071151616b18fc79d850ce8 


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


Testing
---

sudo make check (Fedora 27)


Thanks,

James Peach

Re: Review Request 66615: Adopted `createTask` helper API in Docker containerizer tests.

[James Peach]

> On April 14, 2018, 12:25 a.m., Gaston Kleiman wrote:
> > Thanks for the cleanup!
> > 
> > I think we could get rid of quite some boilerplate with a helper that 
> > creates a `ContainerInfo::DockerInfo`.

Good point!


- James


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


On April 13, 2018, 11:57 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66615/
> ---
> 
> (Updated April 13, 2018, 11:57 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and 
> Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Docker containerizer tests were manually creating the `TaskInfo`
> for the tasks they wanted to launch. We can remove some of that
> boilerplate by adopting the `createTask` helper.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 847258daadf3c37d9071151616b18fc79d850ce8 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
> 
> 
> Diff: https://reviews.apache.org/r/66615/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 27).
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Andrew Schwartzmeyer]

> On April 13, 2018, 6:39 p.m., Mesos Reviewbot Windows wrote:
> > FAIL: Some of the unit tests failed. Please check the relevant logs.
> > 
> > Reviews applied: `['66616']`
> > 
> > Failed command: `Start-MesosCITesting`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66616
> > 
> > Relevant logs:
> > 
> > - 
> > [mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66616/logs/mesos-tests-cmake-stdout.log):
> > 
> > ```
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
> >  warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss 
> > of data 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
> >  warning C4090: 'function': different 'const' qualifiers 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
> >  warning C4716: 'pthread_cond_broadcast': must return a value 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
> >  warning C4716: 'pthread_cond_wait': must return a value 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
> >  warning C4996: 'fopen': This function or variable may be unsafe. Consider 
> > using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
> > See online help for details. 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
> >  warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
> > loss of data 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
> >  warning C4267: 'function': conversion from 'size_t' to 'int', possible 
> > loss of data 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
> >  warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
> > loss of data 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
> >  warning C4267: 'function': conversion from 'size_t' to 'int', possible 
> > loss of data 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
> >  warning C4267: 'function': conversion from 'size_t' to 'int', possible 
> > loss of data 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
> >  warning C4267: 'function': conversion from 'size_t' to 'int', possible 
> > loss of data 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543):
> >  warning C4996: 'strcpy': This function or variable may be unsafe. Consider 
> > using strcpy_s instead. To disable deprecation, use 
> > _CRT_SECURE_NO_WARNINGS. See online help for details. 
> > [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj]
> >  [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
> >  
> > d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548):
> >  warning C4996: 'fopen': This function or variable may be unsafe. Consider 
> > using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
> > See online help for details. 
> > 

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

[Andrew Schwartzmeyer]

> On April 12, 2018, 9:44 a.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 32-35 (patched)
> > 
> >
> > Nit: can this be moved down so it goes "no options", "grpc option", 
> > "java option", "internal option."
> 
> Chun-Hung Hsiao wrote:
> I put it in this order because this is from the 3rdparty bundle and 
> (although there is no public Mesos proto that uses CSI for now) this opens 
> the possiblity to let a public Mesos proto to use CSI.

Gotcha.


- Andrew


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


On April 11, 2018, 6:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 11, 2018, 6:39 p.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-8724, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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 16, 2018, 9:42 a.m.)


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


Changes
---

Rebase after dependent patch change.


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/9/

Changes: https://reviews.apache.org/r/66050/diff/8-9/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

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

- Mesos Reviewbot


On April 15, 2018, 3:49 a.m., Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated April 15, 2018, 3:49 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-8788
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add alg RS256 support for JWT generator and validator.
> 
> Currently, the JWT library only supports unsecured and HS256 tokens. I 
> implemented RS256 to use asymmetrical keys so that Mesos can use it at some 
> point.
> 
> https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/1/
> 
> 
> Testing
> ---
> 
> I've added the same tests than the ones for HS256 (i.e., validation in 
> following cases: bad header, bad payload, unknown alg, unsupported alg, valid 
> token etc.. and creation of a valid token).
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

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/#review201213
---



FAIL: Some of the unit tests failed. Please check the relevant logs.

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

Failed command: `Start-MesosCITesting`

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

Relevant logs:

- 
[mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635/logs/mesos-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543):
 warning C4996: 'strcpy': This function or variable may be unsafe. Consider 
using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569):
 warning 

Review Request 66634: Explicitly marked functions `override` in cgroup subsystems.

[Benjamin Bannier]

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

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
---

This patch adds `override` keywords to functions overriding `virtual`
base class functions. This explicitness helps in a subsequent patch in
refactoring the `Subsystem` base class interface.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
282a76189579d3ddd61f4aad210ce8e876ff0c25 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
e4977da476a04a708a2e9b9dcc5a2f85da5867f4 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.hpp 
b5d712a8eb8ba74092184024e3b40ad9ba1b7584 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.hpp 
27407133e7315dccf1efe2440cc1bf79c51e7dca 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
d79fda3cf852e50e75dbeeeaeae50b1f471a55a4 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
bcaa16142c4e9882dea88e70095cfb7f223401ef 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.hpp 
002c689503d45622cba437c851561f5ec7dc8a12 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
14c0e79f045fb826d20e476772e017439977dded 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/pids.hpp 
cb6c91920355d3d5599f8a3cf0ce2ac5eee18d37 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

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

[Benjamin Bannier]

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

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
style 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/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier

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

[Benno Evers]

> On April 16, 2018, 10:32 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 286 (patched)
> > 
> >
> > Can we add a subfolder with current process pid to address scenarios 
> > where multiple libprocess processes are profiled on the same machine?

The `XX`-part already makes it unique per process.


> On April 16, 2018, 10:32 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 325-328 (patched)
> > 
> >
> > This can also happen if there are no useful data in the profile. Is it 
> > what you mean by "empty"?

Yes, that's what I meant. Reworded to "contains data".


> On April 16, 2018, 10:32 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 667-672 (patched)
> > 
> >
> > Current logic allows users to download profiles from the _previous_ run 
> > while the _current_ run is active. Is it done on purpose? 
> > 
> > I can imagine users expecting download to return intermediate results 
> > of the _current_ run, hence I'd rather either clean the state before 
> > starting a new run or disallow implicit, i.e., without id, download request 
> > if a profiling is running.

Ok, added as a special case.


- Benno


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


On April 10, 2018, 2:39 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63368/
> ---
> 
> (Updated April 10, 2018, 2:39 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> 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 gives developers an easy method to collect and
> access heap profiles which report which pieces of code were
> responsible for allocating memory.
> 
> 
> 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 
>   CHANGELOG c02d56d6612c432bb079a15fde84cb30d7455971 
> 
> 
> Diff: https://reviews.apache.org/r/63368/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 66039: Check both disk and inode usage when slaves schedule gc.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66038, 66039]

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 14, 2018, 11:02 a.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66039/
> ---
> 
> (Updated April 14, 2018, 11:02 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8665
> https://issues.apache.org/jira/browse/MESOS-8665
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check both disk and inode usage when slaves schedule gc.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
> 
> 
> Diff: https://reviews.apache.org/r/66039/diff/3/
> 
> 
> Testing
> ---
> 
> No new tests are added and all "make check" tests are passed.
> 
> 
> Thanks,
> 
> fei long
> 
>

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/#review201184
---




3rdparty/libprocess/src/memory_profiler.cpp
Lines 286 (patched)


Can we add a subfolder with current process pid to address scenarios where 
multiple libprocess processes are profiled on the same machine?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 319 (patched)


Space between `>%` please



3rdparty/libprocess/src/memory_profiler.cpp
Lines 325-328 (patched)


This can also happen if there are no useful data in the profile. Is it what 
you mean by "empty"?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 326 (patched)


Add period after error.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 667-672 (patched)


Current logic allows users to download profiles from the _previous_ run 
while the _current_ run is active. Is it done on purpose? 

I can imagine users expecting download to return intermediate results of 
the _current_ run, hence I'd rather either clean the state before starting a 
new run or disallow implicit, i.e., without id, download request if a profiling 
is running.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 983 (patched)


Should not it be `jemallocRawProfile.path()`?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 1026 (patched)


Can you please use the jsonify library? Example: 
https://github.com/apache/mesos/blob/bd688e4cf9f6f35c9d75aad50b99e1fdeb8104da/src/master/http.cpp#L1541-L1585



3rdparty/libprocess/src/memory_profiler.cpp
Lines 1031-1032 (patched)


Let's print the directory where profiles are stored as well.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 1075-1077 (patched)


s/build/build_options


- Alexander Rukletsov


On April 10, 2018, 2:39 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63368/
> ---
> 
> (Updated April 10, 2018, 2:39 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> 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 gives developers an easy method to collect and
> access heap profiles which report which pieces of code were
> responsible for allocating memory.
> 
> 
> 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 
>   CHANGELOG c02d56d6612c432bb079a15fde84cb30d7455971 
> 
> 
> Diff: https://reviews.apache.org/r/63368/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

[Jan Schlicht]

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



Looks great, thanks for adding this feature!

Most issues are related to style, otherwise there isn't much that would need 
bigger changes.


3rdparty/libprocess/include/process/jwt.hpp
Line 48 (original), 50 (patched)


Please add 'RS256' to the supported algorithms.



3rdparty/libprocess/src/jwt.cpp
Line 30 (original), 28-31 (patched)


Please sort these usings.



3rdparty/libprocess/src/jwt.cpp
Lines 262 (patched)


Break before `const string& token`, i.e. every function argument on a 
separate line.



3rdparty/libprocess/src/jwt.cpp
Lines 265-269 (patched)


This shouldn't really happen, as we're making sure in `pemToRSA` that this 
isn't `nullptr`. Hence we should consider a violation of this fatal and do a
```
CHECK_NOTNULL(publicKey.get());
```
instead.



3rdparty/libprocess/src/jwt.cpp
Lines 312-314 (patched)


This should include a reason (e.g. the OpenSSL error) why the verification 
failed. See my comment on `verify_rsa_sha256` below on how this could be 
achieved by returning a `Try`.



3rdparty/libprocess/src/jwt.cpp
Line 306 (original)


Keep this line.



3rdparty/libprocess/src/ssl/utilities.cpp
Line 371 (original), 371 (patched)


Insert an empty line here, were using 2 lines between functions. Here and 
below.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 375-384 (patched)


Indent with 2 spaces. Here and in the functions below.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 386 (patched)


Insert a line.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 390 (patched)


Insert a line.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 394 (patched)


Insert a line.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 398-399 (patched)


Let's not use `malloc` here. A common pattern in Mesos would be
```
std::vector signatureData;
signatureData.reserve(RSA_size(privateKey.get());
```



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 416 (patched)


Let's include the OpenSSL error string here.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 421 (patched)


Insert a line.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 422 (patched)


Let's have this function return a `Try` and return an error if 
`RSA_verify != 1` and construct an error message including the actual OpenSSL 
error like it's done in `process/openssl.cpp` or above in 
`generate_hmac_sha256`.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 442 (patched)


Remove this line.



3rdparty/libprocess/src/tests/jwt_keys.hpp
Lines 21 (patched)


Please use `const char PRIVATE_KEY[]` instead of `const std::string 
PRIVATE_KEY` as this is the convention used in Mesos code.



3rdparty/libprocess/src/tests/jwt_keys.hpp
Lines 51 (patched)


`const char PUBLIC_KEY[]`



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 26-27 (patched)


Because `jwt_tests.cpp` is the only unit using these keys, we could just 
define them here directly. Though it's nice to have this large block of text in 
a separate file, so this isn't an issue for me.



3rdparty/libprocess/src/tests/jwt_tests.cpp
Line 29 (original), 31-35 (patched)


Please sort these usings.



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 50 (patched)


Two lines between functions.



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 52 (patched)


Indent with 4 spaces.



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 61 (patched)


Dito.



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 63 (patched)

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

[Clément Michaud]

> On avr. 16, 2018, 9:25 matin, Alexander Rojas wrote:
> > I haven't review it very thoroughly yet, but I found something that needs 
> > to be fixed. SSL support is an optional configurable feature (`--with-ssl` 
> > in the configure options). Since your patch depends on SSL, you need the 
> > guards to enable the feature just when SSL is enabled.

Hello Alexander, thanks for the review.
Actually, the code is compiled only when the flag is set to true. Check 
Makefile.am, `src/jwt.cpp` and `src/ssl/utilities.cpp` are in `if ENABLE_SSL` 
block.


- Clément


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


On avr. 14, 2018, 10:19 après-midi, Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated avr. 14, 2018, 10:19 après-midi)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-8788
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add alg RS256 support for JWT generator and validator.
> 
> Currently, the JWT library only supports unsecured and HS256 tokens. I 
> implemented RS256 to use asymmetrical keys so that Mesos can use it at some 
> point.
> 
> https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/1/
> 
> 
> Testing
> ---
> 
> I've added the same tests than the ones for HS256 (i.e., validation in 
> following cases: bad header, bad payload, unknown alg, unsupported alg, valid 
> token etc.. and creation of a valid token).
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

[Alexander Rojas]

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



I haven't review it very thoroughly yet, but I found something that needs to be 
fixed. SSL support is an optional configurable feature (`--with-ssl` in the 
configure options). Since your patch depends on SSL, you need the guards to 
enable the feature just when SSL is enabled.

- Alexander Rojas


On April 15, 2018, 12:19 a.m., Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated April 15, 2018, 12:19 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-8788
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add alg RS256 support for JWT generator and validator.
> 
> Currently, the JWT library only supports unsecured and HS256 tokens. I 
> implemented RS256 to use asymmetrical keys so that Mesos can use it at some 
> point.
> 
> https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/1/
> 
> 
> Testing
> ---
> 
> I've added the same tests than the ones for HS256 (i.e., validation in 
> following cases: bad header, bad payload, unknown alg, unsupported alg, valid 
> token etc.. and creation of a valid token).
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>