Re: Review Request 63023: Added a test CSI plugin.

[Chun-Hung Hsiao]

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

(Updated Nov. 28, 2017, 6 a.m.)


Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
---

Rewrote the test plugin with the new CSI spec.


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


Repository: mesos


Description
---

The test CSI plugin would just create subdirectories under its working
directories to mimic the behavior of creating volumes, then bind-mount
those volumes to mimic publish.


Diffs (updated)
-

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/csi/utils.hpp PRE-CREATION 
  src/csi/utils.cpp PRE-CREATION 
  src/examples/test_csi_plugin.cpp PRE-CREATION 


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

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


Testing
---

make tests


Thanks,

Chun-Hung Hsiao

Re: Review Request 63022: Imported resources from CSI plugins in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated Nov. 28, 2017, 5:59 a.m.)


Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
---

Keeps resource with persistence even if there is insufficient capacity to avoid 
data loss.


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


Repository: mesos


Description
---

The following lists the steps to import resources from a CSI plugin:
1. Launch the node plugin
  1.1 GetSupportedVersions
  1.2 GetPluginInfo
  1.3 NodeProbe
  1.4 GetNodeID
2. Launch the controller plugin
  2.1 GetSuportedVersions
  2.2 GetPluginInfo
  2.3 ControllerProbe
  2.3 GetControllerCapabilities
3. Get preprovisioned volumes
  3.1 ListVolumes
  3.2 ValidateVolumeCapabilities for each volume
4. GetCapacity for each profile
5. Reconcile with the checkpointed state
6. Report the reconciled resources through UPDATE_STATE


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
49c042cdb1837860aaedde2e48f318ed5ac8b1d1 


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

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


Testing
---


Thanks,

Chun-Hung Hsiao

Review Request 64109: Renamed secret key to JWT secret key in the code base.

[Jie Yu]

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

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


Repository: mesos


Description
---

Renamed secret key to JWT secret key in the code base.


Diffs
-

  src/common/http.hpp 505c6d7fb1c94d2b7a8f9bed262826b935c36b00 
  src/common/http.cpp 226fed440e0bdf4eb87cd5a4a9773105d62bea8a 
  src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 


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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 64028: Removed duplicate resources conversions.

[Benjamin Mahler]

> On Nov. 22, 2017, 7:05 p.m., Benjamin Mahler wrote:
> > Should we be implementing a += overload against 
> > `RepeatedPtrField` instead of doing this? Would that be more 
> > efficient than doing a conversion then adding? I'm also hoping to avoid 
> > code having to this type of conversion, ideally :)
> 
> Dmitry Zhuk wrote:
> There's definitely some room for improvements in `Resources`, and `+=` 
> overload could save some CPU cycles, but not in this case. It will not be as 
> efficient here as having explicit conversion followed by more than one 
> `+=`/`<<`. In current approach we don't trust data in protobuf, and we must 
> do validation (see `Resources::operator+=(const Resource_& that)`, invoked 
> from `Resources(const RepeatedPtrField&)`). So in
> ```
> const Resources r = ...;
> r1 += r;
> r2 += r;
> ```
> we do validation only once during conversion, and `+=(const Resources&)` 
> bypasses validation since `Resources` are known to be valid.
> However `+=(const RepeatedPtrField&)` overload will have to do 
> validatation on each call, so we will validate data twice.

Ah yes, thanks for the clarification! Can you mention this in the description 
and the code?

Also, if there was a performance impact on the benchmark, could you include it 
for posterity?


- Benjamin


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


On Nov. 22, 2017, 1:24 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64028/
> ---
> 
> (Updated Nov. 22, 2017, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `RepeatedPtrField` can be implicitly converted to `Resources`,
> leading to hidden multiple resources conversions on performance-critical
> paths in master. For example, `operator +=` relies on implicit
> conversion, when invoked with `RepeatedPtrField` argument.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
> 
> 
> Diff: https://reviews.apache.org/r/64028/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64107: Renamed agent flag '--executor_secret_key' to '--jwt_secret_key'.

[Jie Yu]

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

(Updated Nov. 28, 2017, 2:46 a.m.)


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


Changes
---

renamed the default test key too.


Repository: mesos


Description
---

Now that this is also re-used for authenticating resource provider
initiated requests for managing standalone containers.


Diffs (updated)
-

  CHANGELOG 08d51dfc77d41fd9277bb0386eea328ca1484e11 
  docs/authentication.md 0abf0696cd953532dc4cd560bd25b148b5006f87 
  docs/configuration/agent.md f1e0681be34d194d66e2b2ecd3f76653a54068e2 
  docs/upgrades.md 0e4015ac9019b018b29f706790124ad22d8ee87d 
  src/slave/flags.hpp 0c02b49c054ef2597298aefe0997beacfc9efeb8 
  src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e 
  src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 
  src/tests/executor_http_api_tests.cpp 
910dbbfc7cf0d2613b4faeacb77e758a4243a4fb 
  src/tests/mesos.hpp e5c2b697a4401f010a7d77ae9acb8a9cbd3846bc 
  src/tests/mesos.cpp 9185b5bf2175be5b0f6b6a03a04e9e5445bf22fd 
  src/tests/slave_authorization_tests.cpp 
11fd0d4e35523eca23a9603ecef0c9d0b65dde38 


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

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 64107: Renamed agent flag '--executor_secret_key' to '--jwt_secret_key'.

[Jie Yu]

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

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


Repository: mesos


Description
---

Now that this is also re-used for authenticating resource provider
initiated requests for managing standalone containers.


Diffs
-

  CHANGELOG 08d51dfc77d41fd9277bb0386eea328ca1484e11 
  docs/authentication.md 0abf0696cd953532dc4cd560bd25b148b5006f87 
  docs/configuration/agent.md f1e0681be34d194d66e2b2ecd3f76653a54068e2 
  docs/upgrades.md 0e4015ac9019b018b29f706790124ad22d8ee87d 
  src/slave/flags.hpp 0c02b49c054ef2597298aefe0997beacfc9efeb8 
  src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e 
  src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 
  src/tests/executor_http_api_tests.cpp 
910dbbfc7cf0d2613b4faeacb77e758a4243a4fb 
  src/tests/mesos.cpp 9185b5bf2175be5b0f6b6a03a04e9e5445bf22fd 


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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 63817: Windows: Enabled more agent tests.

[Akash Gupta]

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




src/tests/slave_tests.cpp
Lines 339 (patched)


Should the #ifdef be inside a function / variable instead of copy + pasting 
into multiple tests?



src/tests/slave_tests.cpp
Lines 675 (patched)


Same here


- Akash Gupta


On Nov. 14, 2017, 11:56 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63817/
> ---
> 
> (Updated Nov. 14, 2017, 11:56 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6714
> https://issues.apache.org/jira/browse/MESOS-6714
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These tests "just worked" once the isolators and command was fixed to be
> Windows-compatible. Particularly, `/bin/echo --author` was replaced with
> an equivalent PowerShell command, and `posix/cpu,posix/mem` was replaced
> with `windows/cpu`.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 61dd940e4452a1dbc6c0361bfc917bdc2dd16be8 
> 
> 
> Diff: https://reviews.apache.org/r/63817/diff/1/
> 
> 
> Testing
> ---
> 
> This is the preliminary work I was doing to start fixing the agent recovery 
> path on Windows. However, since we don't do _any_ checkpointing currently, 
> that bug is much more involved, and I wanted to get these patches up sooner 
> rather than later as they enable a dozen more agent tests.
> 
> This is dependent on the isolator chain because of the cleanup work done in 
> `stout/windows/os.hpp` etc. that this code also touched.
> 
> Test results in non-administrative prompt:
> 
> # mesos-tests
> 
> ```
> [--] Global test environment tear-down
> [==] 755 tests from 78 test cases ran. (156570 ms total)
> [  PASSED  ] 755 tests.
> 
>   YOU HAVE 182 DISABLED TESTS
> ```
> 
> In comparison to `master`:
> 
> ```
> [--] Global test environment tear-down
> [==] 747 tests from 76 test cases ran. (337529 ms total)
> [  PASSED  ] 747 tests.
> 
>   YOU HAVE 187 DISABLED TESTS
> ```
> 
> # libprocess-tests
> 
> ```
> [--] Global test environment tear-down
> [==] 158 tests from 29 test cases ran. (992 ms total)
> [  PASSED  ] 158 tests.
> ```
> 
> # stout-tests
> 
> ```
> [--] Global test environment tear-down
> [==] 259 tests from 43 test cases ran. (5185 ms total)
> [  PASSED  ] 259 tests.
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 61158: Introduced http::Server in process.cpp.

[Benjamin Mahler]

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



I'm quite concerned about maintaining the two code paths for the socket 
manager, and I couldn't quite figure out the code. For example, where does the 
`sockets` map get written to in the case of the http server?


3rdparty/libprocess/src/http_proxy.hpp
Lines 13 (patched)


Hm.. why did you do this?



3rdparty/libprocess/src/process.cpp
Lines 463-480 (original), 465-488 (patched)


Using an #else seems a little cleaner than the two seperate #ifndef/#ifdef 
sections?



3rdparty/libprocess/src/process.cpp
Lines 1137-1147 (original), 1149-1165 (patched)


This seems a little odd, I would expect the http::Server and HttpProxy 
setup here to be in the same location with respect to the rest of 
initialization (e.g. using #idef/#else as well), is there something preventing 
that? Or something requiring them to be this far apart?



3rdparty/libprocess/src/process.cpp
Lines 1235-1273 (patched)


Again, puzzling to find this in a different part of the initialization than 
the HttpProxy case.



3rdparty/libprocess/src/process.cpp
Lines 1243 (patched)


Do you need all this prefixing? Or will SocketImpl::Kind::SSL or shorter be 
resolved?



3rdparty/libprocess/src/process.cpp
Lines 1250-1253 (patched)


The capture by reference scared me, then I realized that nothing is getting 
captured by reference since process_manager is a global? Can you remove the `&`?



3rdparty/libprocess/src/process.cpp
Lines 1261 (patched)


Would be helpful to give some context, like we tried to do for some of the 
other fatal logging:

```
LOG(FATAL) << "Failed to initialize, failed to construct http server: " << 
server.error();
```



3rdparty/libprocess/src/process.cpp
Lines 1313 (patched)


s/shutdown/stop/


- Benjamin Mahler


On Nov. 26, 2017, 10:28 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61158/
> ---
> 
> (Updated Nov. 26, 2017, 10:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Avinash sridharan, and 
> Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using http::Server can be compile time configured via USE_HTTP_SERVER.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http_proxy.hpp 
> 5b6e7e8786ed9eab50cd4c2cfdec455c92d72eca 
>   3rdparty/libprocess/src/http_proxy.cpp 
> f584238aadd86875d7c87736653f394e716397de 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
>   3rdparty/libprocess/src/socket_manager.hpp 
> dd4d111c4ae579420060e547dd12f8f0711c 
> 
> 
> Diff: https://reviews.apache.org/r/61158/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 63816: Windows: Fixed MESOS-6816 to enable `ExecutorEnvironmentVariables`.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Nov. 14, 2017, 11:17 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63816/
> ---
> 
> (Updated Nov. 14, 2017, 11:17 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6816
> https://issues.apache.org/jira/browse/MESOS-6816
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By removing the explicit system environment override code, and instead
> setting the system environment as the default in the `CreateProcess`
> wrapper, the `SlaveTest.ExecutorEnvironmentVariables` can be enabled.
> Note that this also required fixing `os::host_default_path()` and using
> it, because the test used `PATH = /bin`, which breaks on Windows.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 292eecbca246edf068ec8c262aff4f3ce9cd8c67 
>   src/slave/containerizer/mesos/launch.cpp 
> b1584ff292ada5463917792908a13e00859fd1ae 
>   src/tests/slave_tests.cpp 61dd940e4452a1dbc6c0361bfc917bdc2dd16be8 
> 
> 
> Diff: https://reviews.apache.org/r/63816/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63815: Windows: Fixed environment priorities in `shell.hpp`.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Nov. 14, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63815/
> ---
> 
> (Updated Nov. 14, 2017, 11:16 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6816
> https://issues.apache.org/jira/browse/MESOS-6816
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Legacy code had caused the system environment to explicitly override the
> supplied environment, but this is incorrect. The system environment
> should be the default, with any supplied environment variables
> overriding those.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> 5c711837a3adbc89ad8793666e68eff7102566b3 
> 
> 
> Diff: https://reviews.apache.org/r/63815/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63814: Windows: Fixed `os::host_default_path()`.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Nov. 14, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63814/
> ---
> 
> (Updated Nov. 14, 2017, 11:16 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6816
> https://issues.apache.org/jira/browse/MESOS-6816
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the necessary and expected default paths to the default `PATH`
> value represented by this function. Especially needed was the
> `WindowsPowerShell` folder so that tasks using PowerShell can run.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63814/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63813: Windows: Fixed name of default executor.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Nov. 14, 2017, 11:15 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63813/
> ---
> 
> (Updated Nov. 14, 2017, 11:15 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6735
> https://issues.apache.org/jira/browse/MESOS-6735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This path is checked with `os::realpath`, which now correctly errors if
> the path does not exist. Therefore the name must be correct.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 401590605350ef1749b45b32c659afdaa43f43ad 
> 
> 
> Diff: https://reviews.apache.org/r/63813/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63812: Windows: Fixed `os::realpath` to behave like POSIX version.

[Akash Gupta]

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



looks good. just small nit on spacing.


3rdparty/stout/include/stout/os/windows/realpath.hpp
Lines 42 (patched)


4 spaces right?



3rdparty/stout/include/stout/os/windows/realpath.hpp
Line 44 (original), 54 (patched)


Same here


- Akash Gupta


On Nov. 14, 2017, 11:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63812/
> ---
> 
> (Updated Nov. 14, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6735
> https://issues.apache.org/jira/browse/MESOS-6735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing implementation (1) did not resolve symlinks and (2) did not
> check for existence of the path. This resulted in unexpected behaviors
> by users of the function.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/realpath.hpp 
> 81a33bd2708c0871c4f23c12fdd29fc8d35682e3 
> 
> 
> Diff: https://reviews.apache.org/r/63812/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63811: Windows: Added `get_handle_follow` which follows symlinks.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Nov. 14, 2017, 11:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63811/
> ---
> 
> (Updated Nov. 14, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6735
> https://issues.apache.org/jira/browse/MESOS-6735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the opposite of `get_handle_no_follow`. They both use the
> inappropriately named Windows API `CreateFile` to retrieve a `HANDLE` to
> an existing file or directory. This existing `get_handle_no_follow`
> explicitly gets a handle to the symlink itself, and the new
> `get_handle_follow` explicitly resolves symlinks and gets a handle to
> the target.
> 
> This may fail if the target doesn't exist.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp 
> e3d04480cf793482d958ea9571d96a29b6bf09b1 
> 
> 
> Diff: https://reviews.apache.org/r/63811/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 64012: Added new --configuration_compatibility slave flag and implementation.

[Vinod Kone]

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




src/Makefile.am
Lines 1015 (patched)


s/reconfiguration/compatibility/



src/slave/flags.cpp
Lines 462 (patched)


s/configuration_compatibility/reconfiguration_compatibility/



src/slave/reconfiguration.hpp
Lines 28 (patched)


put this in `compatibility` namespace.



src/slave/reconfiguration.hpp
Lines 37 (patched)


`equal`



src/slave/reconfiguration.hpp
Lines 63 (patched)


`additive`



src/slave/reconfiguration.hpp
Lines 67 (patched)


singe blank line.



src/slave/reconfiguration.cpp
Lines 51 (patched)


period at the end.

`Resource`
`Attribute`



src/slave/reconfiguration.cpp
Lines 91 (patched)


"Port" in the next line



src/slave/reconfiguration.cpp
Lines 99-100 (patched)


`previous.domain`
`current.domain`



src/slave/reconfiguration.cpp
Lines 103 (patched)


`resources.size()`



src/slave/reconfiguration.cpp
Lines 108-115 (patched)


pull this above the `switch` statement.



src/slave/reconfiguration.cpp
Lines 171-178 (patched)


ditto. pull this up `switch`.



src/slave/slave.cpp
Lines 6126-6137 (patched)


inline.



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


s/our/the/

s/it will turn/it turns/



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


s/checkConfigurationCompatibility/compatible/



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


remove the bool.



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


s/we keep/the slave keeps/



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


can you inline this?



src/slave/slave.cpp
Lines 6328-6330 (patched)


kill this.



src/slave/slave.cpp
Line 6255 (original), 6336 (patched)


s/mismatch/incompatibility (defined as `equal` in Mesos 1.4 and could be 
`equal` or `additive` in Mesos 1.5)/



src/slave/slave.cpp
Line 6260 (original), 6341 (patched)


just return here.



src/slave/slave.cpp
Line 6286 (original), 6358-6361 (patched)


put this in an else block with a comment on when this block is reached.



src/tests/reconfiguration_tests.cpp
Lines 2 (patched)


Can you add more end to end tests in `slave_recovery_tests.cpp` ?



src/tests/reconfiguration_tests.cpp
Lines 30 (patched)


s/make/create/



src/tests/reconfiguration_tests.cpp
Lines 48 (patched)


2 blank lines.



src/tests/reconfiguration_tests.cpp
Lines 49-50 (patched)


Can you write the correct comment?



src/tests/reconfiguration_tests.cpp
Lines 66 (patched)


2 blank lines.



src/tests/reconfiguration_tests.cpp
Lines 85 (patched)


1 blank line.



src/tests/reconfiguration_tests.cpp
Lines 95 (patched)


Can you make sure to test all cases?


- Vinod Kone


On Nov. 28, 2017, 12:51 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64012/
> ---
> 
> (Updated Nov. 28, 2017, 12:51 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new --configuration_compatibility slave flag and implementation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/slave/flags.hpp 

Re: Review Request 61473: Do not kill non partition aware tasks.

[Ilya Pronin]

> On Nov. 27, 2017, 4:25 p.m., Ilya Pronin wrote:
> > src/master/master.cpp
> > Line 9370 (original), 9322 (patched)
> > 
> >
> > I would suggest moving this closer to `newTaskState` selection logic 
> > for readability.
> 
> Megha Sharma wrote:
> I feel its more readable if we populate the value for `bool unreachable` 
> just before its usage but I am open to moving it if you feel strongly about 
> it.

Not stongly, but when I was reading this part I was like "This looks weird, why 
do we only check for `TASK_GONE_BY_OPERATOR` here? What about other terminal 
states?". Then I scrolled up and found that `newTaskState` can only be 
`TASK_LOST`, `TASK_UNREACHABLE` or `TASK_GONE_BY_OPERATOR`.


- Ilya


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


On Nov. 27, 2017, 4:56 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> ---
> 
> (Updated Nov. 27, 2017, 4:56 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including
> the ones that are still registered which was problematic because
> the offer from this agent could still go to the same framework which
> could then launch new tasks. The agent would then receive tasks
> of the same framework and ignore them because it thinks the
> framework is shutting down. The framework is not shutting down of
> course, so from the master and the scheduler's perspective the task
> is pending in STAGING forever until the next agent reregistration,
> which could happen much later. This commit fixes the problem by
> not shutting down the non-partition aware frameworks on such an
> agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
> 
> 
> Diff: https://reviews.apache.org/r/61473/diff/22/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 64074: Added ENABLE_HTTP_SERVER option for cmake.

[Benjamin Mahler]

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


Fix it, then Ship it!




Why just cmake?


cmake/CompilationConfigure.cmake
Lines 100-103 (patched)


For someone that doesn't know the details, this naming seems a little 
unintuitive, i.e. doesn't libprocess already have an http server?

I'm also a little worried about how long it will take us to migrate to the 
new http server code. Is there a plan in place for the transition of the 
default? Why not transition the default to TRUE to begin with? Is the primary 
concern the lack of benchmarks? Would be good to document this for posterity in 
some of these commits.


- Benjamin Mahler


On Nov. 26, 2017, 10:28 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64074/
> ---
> 
> (Updated Nov. 26, 2017, 10:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Avinash sridharan, and 
> Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ENABLE_HTTP_SERVER option for cmake.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake deb574200bceb605a60b536a396c98d9247c6018 
> 
> 
> Diff: https://reviews.apache.org/r/64074/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 63385: Added utility functions for volume attributes and printing CSI messages.

[Chun-Hung Hsiao]

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

(Updated Nov. 28, 2017, 12:57 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Removed comparison for `GetPluginInfoResponse`, and made a generic output 
operator for all messages defined in namespace `::csi`.


Summary (updated)
-

Added utility functions for volume attributes and printing CSI messages.


Repository: mesos


Description (updated)
---

Added utility functions for volume attributes and printing CSI messages.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 6f991e86e46512d5a2bc4e67e160189fccb77f6a 
  src/common/protobuf_utils.cpp c0ff306ae6c16cbba6fd08469b639b9f906c672b 
  src/csi/utils.hpp PRE-CREATION 
  src/csi/utils.cpp PRE-CREATION 
  src/tests/protobuf_utils_tests.cpp 2be26a5f467be85ec0ad28dcd8202e603e4a9d5e 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 61473: Do not kill non partition aware tasks.

[Megha Sharma]

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

(Updated Nov. 28, 2017, 12:56 a.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Master will not kill the tasks for non-Partition aware frameworks
when an unreachable agent re-registers with the master.
Master used to send a ShutdownFrameworkMessages to the agent
to kill the tasks from non partition aware frameworks including
the ones that are still registered which was problematic because
the offer from this agent could still go to the same framework which
could then launch new tasks. The agent would then receive tasks
of the same framework and ignore them because it thinks the
framework is shutting down. The framework is not shutting down of
course, so from the master and the scheduler's perspective the task
is pending in STAGING forever until the next agent reregistration,
which could happen much later. This commit fixes the problem by
not shutting down the non-partition aware frameworks on such an
agent.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 


Diff: https://reviews.apache.org/r/61473/diff/22/

Changes: https://reviews.apache.org/r/61473/diff/21-22/


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 63060: Added utility functions and structures for CSI version and capabilities.

[Chun-Hung Hsiao]

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

(Updated Nov. 28, 2017, 12:56 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Changes
---

Removed the `csi::` prefix. Since the helper functions are moved into the 
`::csi` namespace, the prefix is no longer needed.


Repository: mesos


Description
---

This patch adds some helper structures and functions for CSI protobuf.
The comparison and output operators for `csi::Version` are declared in
the `::csi` namespace so they can find it through ADL. Also, it exposes
`::csi` in `mesos::csi` in `spec.hpp` instead of `client.hpp`.


Diffs (updated)
-

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/csi/client.hpp a5571f40eba60a07154a7ef3ebe36c925629ac63 
  src/csi/spec.hpp 60e40e05d81c9bf649459e29479838dd80daa3aa 
  src/csi/utils.hpp PRE-CREATION 
  src/csi/utils.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/63060/diff/5-6/


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 64012: Added new --configuration_compatibility slave flag and implementation.

[Benno Evers]

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

(Updated Nov. 28, 2017, 12:51 a.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added new --configuration_compatibility slave flag and implementation.


Diffs (updated)
-

  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/slave/flags.hpp 0c02b49c054ef2597298aefe0997beacfc9efeb8 
  src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e 
  src/slave/reconfiguration.hpp PRE-CREATION 
  src/slave/reconfiguration.cpp PRE-CREATION 
  src/slave/slave.hpp 40442f25ae1b223380e11f8602e6256c9203ef47 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/tests/reconfiguration_tests.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 61157: Refactored ProcessManager::handle for future use with http::Server.

[Benjamin Mahler]

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


Ship it!




Coming back to this change without context, it took some time to realize that 
this change was primarily about returning a `Future` from 
`ProcessManager::handle`. A commit description for posterity would be helpful 
IMO.


3rdparty/libprocess/src/process.cpp
Lines 829-831 (patched)


Looks like we can std::move in the future as well, but not until dmitry's 
patches land, not sure if you want to put down a TODO for this.



3rdparty/libprocess/src/process.cpp
Lines 2641-2642 (original), 2608-2609 (patched)


Can this be removed?


- Benjamin Mahler


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 64008: Activated AGENT_UPDATE master capability.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 28, 2017, 12:48 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64008/
> ---
> 
> (Updated Nov. 28, 2017, 12:48 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Activated AGENT_UPDATE master capability.
> 
> 
> Diffs
> -
> 
>   src/master/constants.cpp PRE-CREATION 
>   src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
> 
> 
> Diff: https://reviews.apache.org/r/64008/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63810: Windows: Added internal `fullpath` API to normalize paths.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Nov. 14, 2017, 11:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63810/
> ---
> 
> (Updated Nov. 14, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6735
> https://issues.apache.org/jira/browse/MESOS-6735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This explicitly does not check for existence, nor does it follow
> symlinks. It simply normalizes a given path to an absolute path. This
> removes the `reparsepoint.hpp` and `symlink.hpp` dependency os
> `os::realpath`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp 
> e3d04480cf793482d958ea9571d96a29b6bf09b1 
>   3rdparty/stout/include/stout/internal/windows/symlink.hpp 
> b9cf1229ec0b6c70fe9b9d358e867e91e475dfaa 
>   3rdparty/stout/include/stout/windows/fs.hpp 
> d7b883ca1f3f8972e1bf9fbd211cc564c7c30f14 
> 
> 
> Diff: https://reviews.apache.org/r/63810/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 64008: Activated AGENT_UPDATE master capability.

[Benno Evers]

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

(Updated Nov. 28, 2017, 12:48 a.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Activated AGENT_UPDATE master capability.


Diffs (updated)
-

  src/master/constants.cpp PRE-CREATION 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64101: Skip registry update if SlaveInfo didn't change.

[Vinod Kone]

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




src/master/master.cpp
Lines 6530-6536 (patched)


Expand the comment here mentioning about the performance hit of enquing 
dispatches to registrar after failover.


- Vinod Kone


On Nov. 28, 2017, 12:37 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64101/
> ---
> 
> (Updated Nov. 28, 2017, 12:37 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Skip registry update if SlaveInfo didn't change.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
> 
> 
> Diff: https://reviews.apache.org/r/64101/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

[Jiang Yan Xu]

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

(Updated Nov. 27, 2017, 4:45 p.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

In `updateFramework`, we currently treat frameworks suppressing a role the same 
way as frameworks moving off a role and this could lead to the framework being 
removed from the framework sorter, which is unexpected. We should only remove 
the framework from a framework sorter when it's moving away from the 
corresponding role.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
5ce9ceaa3a5f84a1e076d45448863c418531cc2b 
  src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 


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

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


Testing
---

make check.

Added a test.


Thanks,

Jiang Yan Xu

Re: Review Request 64101: Skip registry update if SlaveInfo didn't change.

[Vinod Kone]

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



Write a test to ensure registry is not updated when there is no change.

- Vinod Kone


On Nov. 28, 2017, 12:37 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64101/
> ---
> 
> (Updated Nov. 28, 2017, 12:37 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Skip registry update if SlaveInfo didn't change.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
> 
> 
> Diff: https://reviews.apache.org/r/64101/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64101: Skip registry update if SlaveInfo didn't change.

[Vinod Kone]

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




src/master/master.cpp
Lines 6530-6536 (patched)


I would just call `__registerSlave` directly here. But ask @mpark for 
advice.


- Vinod Kone


On Nov. 28, 2017, 12:37 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64101/
> ---
> 
> (Updated Nov. 28, 2017, 12:37 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Skip registry update if SlaveInfo didn't change.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
> 
> 
> Diff: https://reviews.apache.org/r/64101/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

[Jiang Yan Xu]

> On Nov. 21, 2017, 10:27 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 430-437 (original), 431-438 (patched)
> > 
> >
> > Do we need a lambda here?

I kept the original use of lambdas. 

I think the benefit of labmdas here is more about readability: compared to 

```
set removedRoles = oldRoles;
foreach (const string& role, newRoles) {
  result.erase(role);
}
```

The constness and construction of the variable is clearly isolated into a small 
block in a long method with many similar variables/code blocks.

However as pointed out by the TODO, we should probably just implement a set 
difference operator so all these become one-liners. I'll do it later.


> On Nov. 21, 2017, 10:27 a.m., Alexander Rukletsov wrote:
> > src/tests/scheduler_tests.cpp
> > Lines 1554-1556 (patched)
> > 
> >
> > How about something like this:
> > // This test verifies that if a framework decides to suppress its roles 
> > on reregisteration, no offers will be made.

The main difference between `NoOffersOnReregistrationWithAllRolesSuppressed` 
and `NoOffersWithAllRolesSuppressed` is the initial condition "a framework 
initially with no roles suppressing offers". Although your suggestion implies 
it, I was emphasizing it. How about:

```
This test verifies that if a framework (initially with no roles suppressed) 
decides to suppress offers for its roles on reregisteration, no offers will be 
made.
```

It's probably simpler to read than my original version.


- Jiang Yan


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


On Nov. 15, 2017, 11:05 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 15, 2017, 11:05 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f0fa7754e5968288edb15929efc9d244b177 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 64009: Added new UpdateSlave registry operation.

[Benjamin Mahler]

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




src/master/registry_operations.cpp
Lines 64-70 (patched)


Hm.. couldn't we normalize the info resources in the agent and master 
rather than leave it in the old format? Have you thought through that?


- Benjamin Mahler


On Nov. 27, 2017, 11:56 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64009/
> ---
> 
> (Updated Nov. 27, 2017, 11:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This operation can be used to update the stored state
> of an existing, admitted slave.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
>   src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
>   src/master/registry_operations.hpp PRE-CREATION 
>   src/master/registry_operations.cpp PRE-CREATION 
>   src/tests/registrar_tests.cpp b0fc5f64dbaf841a548b1e19c90ed47bd1248872 
> 
> 
> Diff: https://reviews.apache.org/r/64009/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 64101: Skip registry update if SlaveInfo didn't change.

[Benno Evers]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Skip registry update if SlaveInfo didn't change.


Diffs
-

  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Vinod Kone]

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




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


If the slave state was changed,



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


back ticks around `slave->info` and `slaveInfo`



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


s/the slave/`slave`/


- Vinod Kone


On Nov. 28, 2017, 12:28 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64011/
> ---
> 
> (Updated Nov. 28, 2017, 12:28 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When an agent reregisters, the master will now always update
> the agent information it holds in memory, and will write any
> changes back to the registry if necessary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
> 
> 
> Diff: https://reviews.apache.org/r/64011/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64098: Send status updates when agent re-registers.

[Megha Sharma]

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

(Updated Nov. 28, 2017, 12:29 a.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Repository: mesos


Description
---

Master will send task status updates to frameworks when an agent
re-registers.


Diffs
-

  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/master_allocator_tests.cpp 3400d70bb0ba564eac43c4639eee0efd4d8059e6 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
  src/tests/persistent_volume_tests.cpp 
acfeac16884b00581a3523607ff26f44f6dca53a 
  src/tests/slave_recovery_tests.cpp c864aa92d9ff128a89dbc25653385de25653f56a 
  src/tests/upgrade_tests.cpp 7f434dbba858f636719eec24e92b306b76430c4c 


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


Testing (updated)
---

with make check


Thanks,

Megha Sharma

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Benno Evers]

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

(Updated Nov. 28, 2017, 12:28 a.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

When an agent reregisters, the master will now always update
the agent information it holds in memory, and will write any
changes back to the registry if necessary.


Diffs (updated)
-

  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 63971: Defined a module interface for translating volume profiles.

[Joseph Wu]

> On Nov. 27, 2017, 3:14 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/resource_provider/volume_profile.hpp
> > Lines 28 (patched)
> > 
> >
> > Currently the CSI spec is internal (under `src/`). Do we want to expose 
> > it as a public interface?

Yeah, I have that in a separate patch (not yet posted).  I didn't want to add 
build changes in this review.


> On Nov. 27, 2017, 3:14 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/resource_provider/volume_profile.hpp
> > Lines 86 (patched)
> > 
> >
> > It seems to me that the purpose of this function is that SLRP can 
> > request the capabilities/params for a given profile dynamically. The 
> > alternative is to have a function to provide a static map between profile 
> > names and capabilities/params, and SLRP calls it once during startup. The 
> > advantage of this function over the alternative is that when a profile is 
> > dynamically generated, there is no need to restart SLRP.
> > 
> > However, when a new profile is generated, SLRP still needs to be 
> > notified so that it can report the RAW resource for the new profile. Since 
> > there is not callback mechanism for the module, we still need to restart 
> > SLRP so that it will report the RAW resource for the new profile.
> > 
> > So I'd suggest that we have another function that returns a map from 
> > profile names to capabilities/params that would be called during SLRP 
> > startup instead.

The main idea behind this module is to request profiles right before making 
each CSI affected call.  We are expecting profiles to be mutable at any time, 
so it does not make sense to cache profiles, especially not in the SLRP.  If 
there is any caching, the module implementation will be the one to do so.

This means there will be a possibility for a race between a 
`CreateVolumeRequest` and a `ControllerPublishVolumeRequest`, if the operator 
updates the profiles between the calls.  However, I'm assuming that updating 
profiles will not change existing volumes unless the volume is re-created.


- Joseph


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


On Nov. 27, 2017, 1:33 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63971/
> ---
> 
> (Updated Nov. 27, 2017, 1:33 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This module is currently intended for use by the Storage Local
> Resource Provider (SLRP), but may be used by other components if
> those components use any of the affected Container Storage Interface
> (CSI) requests. The affected calls are listed in the module's comments.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63971/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 63212: Added a findByTarget method for fs::MountInfoTable.

[Jie Yu]

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

(Updated Nov. 28, 2017, 12:28 a.m.)


Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.


Changes
---

Fix a comment.


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


Repository: mesos


Description
---

This method allows us to get the mount table entry that contains the
specified target.


Diffs (updated)
-

  src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
  src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
  src/tests/containerizer/fs_tests.cpp 1022b824045dc8fb6f8bfeb5f7b0dee95c82ff6c 


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

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


Testing
---


Thanks,

Jie Yu

Re: Review Request 61473: Do not kill non partition aware tasks.

[Ilya Pronin]

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



The approach looks good to me.


src/master/http.cpp
Line 320 (original), 320 (patched)


Style: should be `JSON::ArrayWriter* writer`.



src/master/http.cpp
Lines 322 (patched)


Style: `&` should be attached to the type.



src/master/http.cpp
Lines 343-345 (patched)


I may be ignorant of the discussion behind this, but since we don't treat 
these tasks as completed anymore, do we really need to maintain backwards 
compatibility here, in stats and metrics? This kind of hides the real state of 
things.



src/master/master.cpp
Line 9370 (original), 9322 (patched)


I would suggest moving this closer to `newTaskState` selection logic for 
readability.



src/master/master.cpp
Line 10318 (original), 10271-10272 (patched)


Is there any reason not to iterate like this?
```cpp
foreachvalue (const Owned& task, framework->unreachableTasks) {
  // ...
}
```


- Ilya Pronin


On Nov. 27, 2017, 2:08 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> ---
> 
> (Updated Nov. 27, 2017, 2:08 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including
> the ones that are still registered which was problematic because
> the offer from this agent could still go to the same framework which
> could then launch new tasks. The agent would then receive tasks
> of the same framework and ignore them because it thinks the
> framework is shutting down. The framework is not shutting down of
> course, so from the master and the scheduler's perspective the task
> is pending in STAGING forever until the next agent reregistration,
> which could happen much later. This commit fixes the problem by
> not shutting down the non-partition aware frameworks on such an
> agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
> 
> 
> Diff: https://reviews.apache.org/r/61473/diff/20/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 63741: Fixed a bug in devolving framework subscription with suppressed roles.

[Jiang Yan Xu]

> On Nov. 21, 2017, 10:04 a.m., Alexander Rukletsov wrote:
> > src/internal/devolve.cpp
> > Lines 194-196 (original), 194-196 (patched)
> > 
> >
> > What about `evolve()`?
> 
> Jiang Yan Xu wrote:
> I suppose I need to handle it there too since `v1::scheduler::Call 
> evolve(const scheduler::Call& call)` is defined there but I have no idea 
> where this is ever used/useful?

I added the custom converstion to `v1::scheduler::Call evolve(const 
scheduler::Call& call)` with a TODO.


> On Nov. 21, 2017, 10:04 a.m., Alexander Rukletsov wrote:
> > src/internal/devolve.cpp
> > Lines 199-200 (patched)
> > 
> >
> > Maybe a simple `if(){}` for now? Maybe the next special case will not 
> > be type dependent?

SGTM.


> On Nov. 21, 2017, 10:04 a.m., Alexander Rukletsov wrote:
> > src/internal/devolve.cpp
> > Lines 203-204 (patched)
> > 
> >
> > I believe we prefer writing `CopyFrom()` explicitly, no?

I don't think it's estalished as a rule even though we do use CopyFrom most 
often previously. Now that we have upgraded to protobuf 3.5 with move 
semantics, I think we can probably standarize on prefering the assignment 
operator as the choice between copy and move can be determined simply by the 
rvalue-ness of the operand unless copying is explicitly intended. I understand 
that this particular case wouldn't be moved but styling wise it's more 
futre-proof. We should further this discussion. For now I think this is OK as 
this style is already used in the codebase.


- Jiang Yan


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


On Nov. 15, 2017, 10:41 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63741/
> ---
> 
> (Updated Nov. 15, 2017, 10:41 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8200
> https://issues.apache.org/jira/browse/MESOS-8200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also improved logging to show the suppressed roles.
> 
> 
> Diffs
> -
> 
>   src/internal/devolve.cpp 3a02490d734ef4a558b448871f9a1054d832f457 
>   src/master/master.cpp 81192d5ce7c095495a5faa534d4792601e540e0e 
> 
> 
> Diff: https://reviews.apache.org/r/63741/diff/1/
> 
> 
> Testing
> ---
> 
> Tested together with /r/63830/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 64010: Added an optional SlaveInfo parameter to Allocator::updateSlave().

[Vinod Kone]

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




include/mesos/allocator/allocator.hpp
Lines 219 (patched)


lets not take an option here, but just `SlaveInfo&`



src/master/allocator/mesos/allocator.hpp
Lines 106 (patched)


s/reset/remove/



src/master/allocator/mesos/allocator.hpp
Lines 249 (patched)


s/reset/remove/



src/master/allocator/mesos/allocator.hpp
Lines 482-494 (patched)


remove this?



src/master/allocator/mesos/hierarchical.hpp
Lines 548 (patched)


s/resets/removes/



src/master/allocator/mesos/hierarchical.hpp
Lines 550 (patched)


s/reset/remove/



src/master/allocator/mesos/hierarchical.cpp
Lines 662-677 (patched)


```
Option newDomain = info->hasDomain() : info->Domain() : None();

if (previousDomain != newDomain) {
  updated = true;
  slave.domain = newDomain;
}

```



src/master/allocator/mesos/hierarchical.cpp
Lines 1343 (patched)


"Removed all filters for slave "


- Vinod Kone


On Nov. 27, 2017, 11:29 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64010/
> ---
> 
> (Updated Nov. 27, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows callers to inform the allocator that an existing agents
> SlaveInfo changed, probably because it was restarted and reregistered
> with the same id.
> 
> Additionally, this also provides callers with an option to reset
> existing offer filters for a given slave id when the update is
> applied.
> 
> While the existing HierarchicalDRFAllocator only cares about the domain
> and hostname fields, we still have to pass the full SlaveInfo here
> because custom allocators might base their decisions on other fields,
> for example attributes.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> ae122003487ca8956573e993cd3993aa8cc286f1 
>   src/master/allocator/mesos/allocator.hpp 
> 8fa4fdeec4ec64bcd49fc442c230d8684a11cfd9 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c4832b29842330fa57756cd3d4202f265a820f3 
>   src/master/allocator/mesos/hierarchical.cpp 
> cfeeb3bfa3ad7d78ff6e22cfc4adb1f0efa05629 
>   src/tests/allocator.hpp 6a84f1beb86dceb5a5e9bf4615c13a216f3d0905 
>   src/tests/hierarchical_allocator_tests.cpp 
> f0f95ba4f667bf8ea54e985d8cde913a4170d8ff 
> 
> 
> Diff: https://reviews.apache.org/r/64010/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63809: Windows: Fixed symlink code to not need admin privileges.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Nov. 14, 2017, 11:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63809/
> ---
> 
> (Updated Nov. 14, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jie Yu, John Kordich, Joseph Wu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7370
> https://issues.apache.org/jira/browse/MESOS-7370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This new feature of Windows 10 allows us to make symlinks without being
> an administrator. In the future, we may want to do a version check, as
> this is likely to fail on versions of Windows which don't support this
> flag. Resolves MESOS-7370.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp 
> e3d04480cf793482d958ea9571d96a29b6bf09b1 
> 
> 
> Diff: https://reviews.apache.org/r/63809/diff/1/
> 
> 
> Testing
> ---
> 
> Ran `mesos-tests` not in an admin prompt!
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 64009: Added new UpdateSlave registry operation.

[Vinod Kone]

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




src/tests/registrar_tests.cpp
Lines 270 (patched)


period at the end.



src/common/protobuf_utils.hpp
Lines 155 (patched)


can you confirm if already converted resources will result in a no-op?



src/common/protobuf_utils.hpp
Lines 157 (patched)


`isEqualPostReservationRefinement()`


- Vinod Kone


On Nov. 27, 2017, 11:29 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64009/
> ---
> 
> (Updated Nov. 27, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This operation can be used to update the stored state
> of an existing, admitted slave.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
>   src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
>   src/master/registry_operations.hpp PRE-CREATION 
>   src/master/registry_operations.cpp PRE-CREATION 
>   src/tests/registrar_tests.cpp b0fc5f64dbaf841a548b1e19c90ed47bd1248872 
> 
> 
> Diff: https://reviews.apache.org/r/64009/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 64102: Renamed curl target to libcurl, and staging of curl.exe on Windows.

[John Kordich via Review Board]

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

Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Repository: mesos


Description
---

Renamed curl target to libcurl, and staging of curl.exe on Windows.


Diffs
-

  3rdparty/CMakeLists.txt c34b65d5ff306e17c3519667ae303a8678fca43d 


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


Testing
---

Ran all tests on Windows/Linux (for this patch plus #64103 and #64104). No 
related tests failed on either Linux or Windows.


Thanks,

John Kordich

Review Request 64104: Added dependency of curl to agent. Enabled most health check tests.

[John Kordich via Review Board]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added dependency of curl to agent. Enabled most health check tests.


Diffs
-

  src/slave/CMakeLists.txt 6f08f3dc95f3a1408cbef7b8f0b0cc2522558924 
  src/tests/health_check_tests.cpp c0dcba265363f2149b217b189ee5a8e925e40ea1 


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


Testing
---

There are two health check tests which are not enabled yet, because they 
require the IOSwitchboard to be ported first. We can't resolve MESOS-6709 quite 
yet until that is addressed. The rest of the tests pass on both Linux/Windows, 
as mentioned in #64102


Thanks,

John Kordich

Review Request 64103: Changed dependency of curl to libcurl for stout.

[John Kordich via Review Board]

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

Review request for mesos.


Repository: mesos


Description
---

Changed dependency of curl to libcurl for stout.


Diffs
-

  3rdparty/stout/CMakeLists.txt 145b772a4f5da5653b918f746fbfc44e58321e47 


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


Testing
---


Thanks,

John Kordich

Re: Review Request 64007: Added infrastructure to support master capabilities.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 27, 2017, 11:29 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64007/
> ---
> 
> (Updated Nov. 27, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes the necessary constants in the protobuf
> definition as well as various support methods to get
> the current master capabilities.
> 
> Note that the actual feature is implemented in a later
> commit, so the capability is not yet activated on the
> master.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
>   src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
>   src/master/constants.hpp 725680b1e5f1fb9ca87ec5130d0eff72309432ae 
>   src/master/constants.cpp PRE-CREATION 
>   src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
> 
> 
> Diff: https://reviews.apache.org/r/64007/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64006: Moved registry operations into separate header.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/registry_operations.hpp
Lines 28 (patched)


one line only.



src/master/registry_operations.hpp
Lines 124 (patched)


one line only.



src/master/registry_operations.cpp
Lines 239 (patched)


2 lines.


- Vinod Kone


On Nov. 27, 2017, 11:29 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64006/
> ---
> 
> (Updated Nov. 27, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The definition of the various master registry operations is only
> needed when trying to access registry, so they were moved into a
> separate file in order to decrease size and complexity of the
> master.hpp header.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/master/registry_operations.hpp PRE-CREATION 
>   src/master/registry_operations.cpp PRE-CREATION 
>   src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
>   src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
>   src/tests/persistent_volume_tests.cpp 
> acfeac16884b00581a3523607ff26f44f6dca53a 
>   src/tests/reconciliation_tests.cpp 8c43ffd1cd4ffe1b11d67eb0a1f768c736826d91 
>   src/tests/registrar_tests.cpp b0fc5f64dbaf841a548b1e19c90ed47bd1248872 
>   src/tests/slave_tests.cpp a75bb260df223b5b86f31e91eec1b6ba8db00cb2 
> 
> 
> Diff: https://reviews.apache.org/r/64006/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Benno Evers]

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

(Updated Nov. 27, 2017, 11:30 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

When an agent reregisters, the master will now always update
the agent information it holds in memory, and will write any
changes back to the registry if necessary.


Diffs (updated)
-

  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64008: Activated AGENT_UPDATE master capability.

[Benno Evers]

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

(Updated Nov. 27, 2017, 11:30 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Activated AGENT_UPDATE master capability.


Diffs (updated)
-

  src/master/constants.cpp PRE-CREATION 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64012: Added new --configuration_compatibility slave flag and implementation.

[Benno Evers]

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

(Updated Nov. 27, 2017, 11:30 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added new --configuration_compatibility slave flag and implementation.


Diffs (updated)
-

  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/slave/flags.hpp 0c02b49c054ef2597298aefe0997beacfc9efeb8 
  src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e 
  src/slave/reconfiguration.hpp PRE-CREATION 
  src/slave/reconfiguration.cpp PRE-CREATION 
  src/slave/slave.hpp 40442f25ae1b223380e11f8602e6256c9203ef47 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/tests/reconfiguration_tests.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64073: Fixed ignored Socket::accept discard when using LibeventSSLSocketImpl.

[Benjamin Mahler]

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




3rdparty/libprocess/src/libevent_ssl_socket.cpp
Line 958 (original), 959-960 (patched)


Can't we just make Queue::get discardable? Seems to have clear semantics. 
And would give us the benefit of returning a discarded future rather than 
convering to Failure as you've done?


- Benjamin Mahler


On Nov. 26, 2017, 10:28 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64073/
> ---
> 
> (Updated Nov. 26, 2017, 10:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Avinash sridharan, and 
> Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently there is no way to discard `LibeventSSLSocketImpl::accept()` 
> because `Queue::get()` isn't discardable. This patch uses an `await()` so 
> that we can discard.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 1c95ebabfefd07aaeb053b965ab8e4550dfccaef 
> 
> 
> Diff: https://reviews.apache.org/r/64073/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 64010: Added an optional SlaveInfo parameter to Allocator::updateSlave().

[Benno Evers]

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

(Updated Nov. 27, 2017, 11:29 p.m.)


Review request for mesos and Vinod Kone.


Summary (updated)
-

Added an optional SlaveInfo parameter to Allocator::updateSlave().


Repository: mesos


Description (updated)
---

This allows callers to inform the allocator that an existing agents
SlaveInfo changed, probably because it was restarted and reregistered
with the same id.

Additionally, this also provides callers with an option to reset
existing offer filters for a given slave id when the update is
applied.

While the existing HierarchicalDRFAllocator only cares about the domain
and hostname fields, we still have to pass the full SlaveInfo here
because custom allocators might base their decisions on other fields,
for example attributes.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
ae122003487ca8956573e993cd3993aa8cc286f1 
  src/master/allocator/mesos/allocator.hpp 
8fa4fdeec4ec64bcd49fc442c230d8684a11cfd9 
  src/master/allocator/mesos/hierarchical.hpp 
2c4832b29842330fa57756cd3d4202f265a820f3 
  src/master/allocator/mesos/hierarchical.cpp 
cfeeb3bfa3ad7d78ff6e22cfc4adb1f0efa05629 
  src/tests/allocator.hpp 6a84f1beb86dceb5a5e9bf4615c13a216f3d0905 
  src/tests/hierarchical_allocator_tests.cpp 
f0f95ba4f667bf8ea54e985d8cde913a4170d8ff 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64007: Added infrastructure to support master capabilities.

[Benno Evers]

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

(Updated Nov. 27, 2017, 11:29 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description (updated)
---

This includes the necessary constants in the protobuf
definition as well as various support methods to get
the current master capabilities.

Note that the actual feature is implemented in a later
commit, so the capability is not yet activated on the
master.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
  src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
  src/master/constants.hpp 725680b1e5f1fb9ca87ec5130d0eff72309432ae 
  src/master/constants.cpp PRE-CREATION 
  src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64009: Added new UpdateSlave registry operation.

[Benno Evers]

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

(Updated Nov. 27, 2017, 11:29 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This operation can be used to update the stored state
of an existing, admitted slave.


Diffs (updated)
-

  src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
  src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
  src/master/registry_operations.hpp PRE-CREATION 
  src/master/registry_operations.cpp PRE-CREATION 
  src/tests/registrar_tests.cpp b0fc5f64dbaf841a548b1e19c90ed47bd1248872 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64006: Moved registry operations into separate header.

[Benno Evers]

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

(Updated Nov. 27, 2017, 11:29 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

The definition of the various master registry operations is only
needed when trying to access registry, so they were moved into a
separate file in order to decrease size and complexity of the
master.hpp header.


Diffs (updated)
-

  src/CMakeLists.txt 4f114184df067f4438c852cff4a40fa01861316a 
  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/master/registry_operations.hpp PRE-CREATION 
  src/master/registry_operations.cpp PRE-CREATION 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
  src/tests/persistent_volume_tests.cpp 
acfeac16884b00581a3523607ff26f44f6dca53a 
  src/tests/reconciliation_tests.cpp 8c43ffd1cd4ffe1b11d67eb0a1f768c736826d91 
  src/tests/registrar_tests.cpp b0fc5f64dbaf841a548b1e19c90ed47bd1248872 
  src/tests/slave_tests.cpp a75bb260df223b5b86f31e91eec1b6ba8db00cb2 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 63997: Added a new allocator method to add resources to agents.

[Benjamin Mahler]

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




include/mesos/allocator/allocator.hpp
Lines 220 (patched)


Hm.. why is this called out? I don't think a caller could depend on this 
for correctness, so it seems better not to put this in the interface 
documentation?



src/master/allocator/mesos/hierarchical.cpp
Lines 689-718 (patched)


Is this copied from elsewhere? If so, can you guys think about how to pull 
it out like we did for `updateSlaveTotal`? Is this `trackAllocatedResources`?


- Benjamin Mahler


On Nov. 27, 2017, 2:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63997/
> ---
> 
> (Updated Nov. 27, 2017, 2:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The added method complements 'Allocator::addSlave'. While in
> 'addSlave' the total agent resources and used resources are passed,
> the method 'addResourceProvider' added here allows to add additional,
> potentially used resources to an existing agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> ae122003487ca8956573e993cd3993aa8cc286f1 
>   src/master/allocator/mesos/allocator.hpp 
> 8fa4fdeec4ec64bcd49fc442c230d8684a11cfd9 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c4832b29842330fa57756cd3d4202f265a820f3 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ce9ceaa3a5f84a1e076d45448863c418531cc2b 
>   src/tests/allocator.hpp 6a84f1beb86dceb5a5e9bf4615c13a216f3d0905 
>   src/tests/hierarchical_allocator_tests.cpp 
> f0f95ba4f667bf8ea54e985d8cde913a4170d8ff 
> 
> 
> Diff: https://reviews.apache.org/r/63997/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 63212: Added a findByTarget method for fs::MountInfoTable.

[Jie Yu]

> On Oct. 25, 2017, 9:26 p.m., James Peach wrote:
> > src/linux/fs.cpp
> > Lines 249 (patched)
> > 
> >
> > So if the target was "/mnt/foobar" and we had a mount called 
> > "/mnt/foo", this would match? Is that what we want here?

Good catch! Fixed.


- Jie


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


On Nov. 27, 2017, 11:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63212/
> ---
> 
> (Updated Nov. 27, 2017, 11:18 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7306
> https://issues.apache.org/jira/browse/MESOS-7306
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This method allows us to get the mount table entry that contains the
> specified target.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
>   src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
>   src/tests/containerizer/fs_tests.cpp 
> 1022b824045dc8fb6f8bfeb5f7b0dee95c82ff6c 
> 
> 
> Diff: https://reviews.apache.org/r/63212/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63212: Added a findByTarget method for fs::MountInfoTable.

[Jie Yu]

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

(Updated Nov. 27, 2017, 11:18 p.m.)


Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.


Changes
---

Addressed comments from James and add tests.


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


Repository: mesos


Description
---

This method allows us to get the mount table entry that contains the
specified target.


Diffs (updated)
-

  src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
  src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
  src/tests/containerizer/fs_tests.cpp 1022b824045dc8fb6f8bfeb5f7b0dee95c82ff6c 


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

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


Testing
---


Thanks,

Jie Yu

Re: Review Request 63971: Defined a module interface for translating volume profiles.

[Chun-Hung Hsiao]

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




include/mesos/resource_provider/volume_profile.hpp
Lines 28 (patched)


Currently the CSI spec is internal (under `src/`). Do we want to expose it 
as a public interface?



include/mesos/resource_provider/volume_profile.hpp
Lines 86 (patched)


It seems to me that the purpose of this function is that SLRP can request 
the capabilities/params for a given profile dynamically. The alternative is to 
have a function to provide a static map between profile names and 
capabilities/params, and SLRP calls it once during startup. The advantage of 
this function over the alternative is that when a profile is dynamically 
generated, there is no need to restart SLRP.

However, when a new profile is generated, SLRP still needs to be notified 
so that it can report the RAW resource for the new profile. Since there is not 
callback mechanism for the module, we still need to restart SLRP so that it 
will report the RAW resource for the new profile.

So I'd suggest that we have another function that returns a map from 
profile names to capabilities/params that would be called during SLRP startup 
instead.


- Chun-Hung Hsiao


On Nov. 27, 2017, 9:33 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63971/
> ---
> 
> (Updated Nov. 27, 2017, 9:33 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This module is currently intended for use by the Storage Local
> Resource Provider (SLRP), but may be used by other components if
> those components use any of the affected Container Storage Interface
> (CSI) requests. The affected calls are listed in the module's comments.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63971/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 63018: Added filesystem layout for local resource providers.

[Chun-Hung Hsiao]

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

(Updated Nov. 27, 2017, 10:14 p.m.)


Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
---

Rebased.


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


Repository: mesos


Description
---

A local resource provide can store checkpoints, whose lifecycles should
be tied to the agent, under
`/meta/slaves//resource_providers///
`.


Diffs (updated)
-

  src/resource_provider/daemon.cpp 80b5c71e266bda2f15d5dfb9b1c15f01c6aa8e93 
  src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
  src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
  src/resource_provider/storage/provider.hpp 
6de88c2329b358fcf48bc39ddda0132170991c3c 
  src/resource_provider/storage/provider.cpp 
49c042cdb1837860aaedde2e48f318ed5ac8b1d1 
  src/slave/paths.hpp 66dfa4544772d78ccc9229dc861da60c79913f24 
  src/slave/paths.cpp b03ffeeed83cb73228cca27769262fb08df38fb5 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 61473: Do not kill non partition aware tasks.

[Megha Sharma]

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

(Updated Nov. 27, 2017, 10:08 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Master will not kill the tasks for non-Partition aware frameworks
when an unreachable agent re-registers with the master.
Master used to send a ShutdownFrameworkMessages to the agent
to kill the tasks from non partition aware frameworks including
the ones that are still registered which was problematic because
the offer from this agent could still go to the same framework which
could then launch new tasks. The agent would then receive tasks
of the same framework and ignore them because it thinks the
framework is shutting down. The framework is not shutting down of
course, so from the master and the scheduler's perspective the task
is pending in STAGING forever until the next agent reregistration,
which could happen much later. This commit fixes the problem by
not shutting down the non-partition aware frameworks on such an
agent.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 


Diff: https://reviews.apache.org/r/61473/diff/20/

Changes: https://reviews.apache.org/r/61473/diff/19-20/


Testing
---

make check


Thanks,

Megha Sharma

Review Request 64098: Send status updates when agent re-registers.

[Megha Sharma]

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

Review request for mesos, James Peach and Jiang Yan Xu.


Repository: mesos


Description
---

Master will send task status updates to frameworks when an agent
re-registers.


Diffs
-

  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 


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


Testing
---


Thanks,

Megha Sharma

Re: Review Request 61473: Do not kill non partition aware tasks.

[Megha Sharma]

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

(Updated Nov. 27, 2017, 10:05 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Summary (updated)
-

Do not kill non partition aware tasks.


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


Repository: mesos


Description (updated)
---

Master will not kill the tasks for non-Partition aware frameworks
when an unreachable agent re-registers with the master.
Master used to send a ShutdownFrameworkMessages to the agent
to kill the tasks from non partition aware frameworks including
the ones that are still registered which was problematic because
the offer from this agent could still go to the same framework which
could then launch new tasks. The agent would then receive tasks
of the same framework and ignore them because it thinks the
framework is shutting down. The framework is not shutting down of
course, so from the master and the scheduler's perspective the task
is pending in STAGING forever until the next agent reregistration,
which could happen much later. This commit fixes the problem by
not shutting down the non-partition aware frameworks on such an
agent.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/master/http.cpp 10084125deb839a9846a4f64d2e433ff02754c02 
  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 


Diff: https://reviews.apache.org/r/61473/diff/19/

Changes: https://reviews.apache.org/r/61473/diff/18-19/


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 61473: Send status updates when agent re-registers.

[Megha Sharma]

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

(Updated Nov. 27, 2017, 10:02 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Split into multiple commits.


Summary (updated)
-

Send status updates when agent re-registers.


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


Repository: mesos


Description (updated)
---

Master will send task status updates to frameworks when an agent
re-registers.


Diffs (updated)
-

  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 


Diff: https://reviews.apache.org/r/61473/diff/18/

Changes: https://reviews.apache.org/r/61473/diff/17-18/


Testing
---

make check


Thanks,

Megha Sharma

Review Request 63971: Defined a module interface for translating volume profiles.

[Joseph Wu]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

This module is currently intended for use by the Storage Local
Resource Provider (SLRP), but may be used by other components if
those components use any of the affected Container Storage Interface
(CSI) requests. The affected calls are listed in the module's comments.


Diffs
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 


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


Testing
---


Thanks,

Joseph Wu

Re: Review Request 63212: Added a findByTarget method for fs::MountInfoTable.

[Jie Yu]

> On Oct. 25, 2017, 9:26 p.m., James Peach wrote:
> > src/linux/fs.cpp
> > Lines 257 (patched)
> > 
> >
> > Did you consider using a `Result` and returning `None()` in this case?

I think not found is really not possible because `/` is always at the root.


- Jie


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


On Nov. 27, 2017, 4:49 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63212/
> ---
> 
> (Updated Nov. 27, 2017, 4:49 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7306
> https://issues.apache.org/jira/browse/MESOS-7306
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This method allows us to get the mount table entry that contains the
> specified target.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
>   src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
> 
> 
> Diff: https://reviews.apache.org/r/63212/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Chun-Hung Hsiao]

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

(Updated Nov. 27, 2017, 9:26 p.m.)


Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.


Diffs (updated)
-

  src/resource_provider/daemon.hpp 8668cd1c4c0be79e8e18371744c9f043129f30c5 
  src/resource_provider/daemon.cpp 80b5c71e266bda2f15d5dfb9b1c15f01c6aa8e93 
  src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
  src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
  src/resource_provider/storage/provider.hpp 
6de88c2329b358fcf48bc39ddda0132170991c3c 
  src/resource_provider/storage/provider.cpp 
49c042cdb1837860aaedde2e48f318ed5ac8b1d1 
  src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 


Diff: https://reviews.apache.org/r/62636/diff/10/

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63732: Reconciled offer operations between agent and master.

[Jie Yu]

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




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


Can you add a comment saying that if the key is None, it means the agent 
default resources that have no resource provider id.



src/master/master.cpp
Lines 7128-7129 (patched)


Please use `Resources::hasResourceProvider` which has more CHECKs.



src/master/master.cpp
Lines 7211-7218 (patched)


i feel this is unnecessary. It should be the responsibility of the agent to 
convert the operation to post refinement format if it is every downgraded 
before checkpointing.

We can add some CHECK here.



src/master/master.cpp
Lines 7231-7244 (patched)


Hum, what if this is a failover master and the UpdateSlaveMessage is a 
result of an LRP re-registration with the agent. In that case, master does not 
know about the operation but the agent does?

Also, i would be future proof and does not check the latest state of the 
operation. It's possible that the operaiton is terminal but still waiting for 
ack from the framework, and master will still maintain that operation. I would 
still check the invariant that if the master knows about the RP, then it should 
know more operation than the agent.

There might be one edge case worth documenting: master receives an ack from 
the framework, and removed the operation and send an Ack message to the agent. 
The agent didn't receive the ack yet before it sends an UpdateSlaveMessage. In 
that case, what we should do? Adding back the operation does not sound right 
because the agent will then delete the operation on receiving ack. Then we have 
an inconsistency between master and agent.



src/master/master.cpp
Lines 7302-7308 (patched)


We do want to support that when a CSI plugin detects a chagne to the total 
resources.



src/master/master.cpp
Lines 7352-7362 (patched)


This should go to addOfferOperation.



src/master/master.cpp
Lines 7389-7403 (patched)


That's `Slave::recoverResources(operation)`?


- Jie Yu


On Nov. 24, 2017, 2:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63732/
> ---
> 
> (Updated Nov. 24, 2017, 2:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8207
> https://issues.apache.org/jira/browse/MESOS-8207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
> 
> 
> Diff: https://reviews.apache.org/r/63732/diff/7/
> 
> 
> Testing
> ---
> 
> `make check`, tested as part of https://reviews.apache.org/r/63843/.
> 
> This patch requires `protobuf::isSpeculativeOperation` from 
> https://reviews.apache.org/r/63751/ which is _not_ part of this review chain 
> (to avoid multiple dependent RRs).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 64096: Implemented the `OfferOperationStatusUpdateManager`.

[Gaston Kleiman]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

This class will handle the offer operation status updates generated by
the agent and by resource providers.


Diffs
-

  src/CMakeLists.txt be212d9316e1b8a605b2f7f2cdaae37a3b0871bf 
  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/status_update_manager/offer_operation_status_update_manager.hpp 
PRE-CREATION 
  src/status_update_manager/offer_operation_status_update_manager.cpp 
PRE-CREATION 
  src/tests/CMakeLists.txt 8997cc0f762aa27decaa24a147a8581549a82e80 
  src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION 


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


Testing
---

This patch addes new tests that passed 5000 times on GNU/Linux.


Thanks,

Gaston Kleiman

Review Request 64095: Added a generic actor to be used by status update managers.

[Gaston Kleiman]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

This actor handles the checkpointing, recovery, and retry of status
updates, and will initially be used by the offer operations status
update manager.


Diffs
-

  src/status_update_manager/status_update_manager_process.hpp PRE-CREATION 


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


Testing
---

Added new tests at the end of the chain. They passed 5000 times on GNU/Linux.


Thanks,

Gaston Kleiman

Review Request 64094: Added the `OfferOperationStatusUpdateRecord` protobuf message.

[Gaston Kleiman]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

This protobuf message is used to checkpoint offer operation status
updates and acknowledgments.


Diffs
-

  src/messages/messages.hpp 2756ebae5e2dccce63030eb7cec55d35445fa97b 
  src/messages/messages.cpp 6029502e8394817a0ab6ee2f63895ad02349bb0d 
  src/messages/messages.proto d7ef68f85a9f8a7702a83a3554c2dedc6d7ee339 


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


Testing
---

Added new tests at the end of the chain. They passed 5000 times on GNU/Linux.


Thanks,

Gaston Kleiman

Review Request 64093: Added operators for offer operation update protobuf classes.

[Gaston Kleiman]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

Added operators for offer operation update protobuf classes.


Diffs
-

  src/messages/messages.hpp 2756ebae5e2dccce63030eb7cec55d35445fa97b 
  src/messages/messages.cpp 6029502e8394817a0ab6ee2f63895ad02349bb0d 


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


Testing
---

Added new tests at the end of the chain. They passed 5000 times on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 64000: Allowed stringification of hashmaps of stringifiable types.

[Benjamin Bannier]

> On Nov. 27, 2017, 7:24 p.m., Jie Yu wrote:
> > include/mesos/type_utils.hpp
> > Line 467 (original), 467 (patched)
> > 
> >
> > Do you also need to update v1/mesos.cpp?

This makes me wonder whether we should just remove these duplicated 
declarations from these mesos-specific headers, and instead move this code into 
`stout/hashmap.hpp`. Does that make sense?


- Benjamin


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


On Nov. 27, 2017, 4 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64000/
> ---
> 
> (Updated Nov. 27, 2017, 4 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed stringification of hashmaps of stringifiable types.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
>   src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
> 
> 
> Diff: https://reviews.apache.org/r/64000/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 63741: Fixed a bug in devolving framework subscription with suppressed roles.

[Jiang Yan Xu]

> On Nov. 21, 2017, 10:04 a.m., Alexander Rukletsov wrote:
> > src/internal/devolve.cpp
> > Lines 194-196 (original), 194-196 (patched)
> > 
> >
> > What about `evolve()`?

I suppose I need to handle it there too since `v1::scheduler::Call evolve(const 
scheduler::Call& call)` is defined there but I have no idea where this is ever 
used/useful?


- Jiang Yan


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


On Nov. 15, 2017, 10:41 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63741/
> ---
> 
> (Updated Nov. 15, 2017, 10:41 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8200
> https://issues.apache.org/jira/browse/MESOS-8200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also improved logging to show the suppressed roles.
> 
> 
> Diffs
> -
> 
>   src/internal/devolve.cpp 3a02490d734ef4a558b448871f9a1054d832f457 
>   src/master/master.cpp 81192d5ce7c095495a5faa534d4792601e540e0e 
> 
> 
> Diff: https://reviews.apache.org/r/63741/diff/1/
> 
> 
> Testing
> ---
> 
> Tested together with /r/63830/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 64000: Allowed stringification of hashmaps of stringifiable types.

[Jie Yu]

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




include/mesos/type_utils.hpp
Line 467 (original), 467 (patched)


Do you also need to update v1/mesos.cpp?


- Jie Yu


On Nov. 27, 2017, 3 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64000/
> ---
> 
> (Updated Nov. 27, 2017, 3 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed stringification of hashmaps of stringifiable types.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
>   src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
> 
> 
> Diff: https://reviews.apache.org/r/64000/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64000: Allowed stringification of hashmaps of stringifiable types.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Nov. 27, 2017, 3 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64000/
> ---
> 
> (Updated Nov. 27, 2017, 3 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed stringification of hashmaps of stringifiable types.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
>   src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
> 
> 
> Diff: https://reviews.apache.org/r/64000/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 63731: Reconciled pending resource provider operations in agent.

[Jie Yu]

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


Fix it, then Ship it!





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


Can you resolve this `FIXME`? (either TODO or NOTE)


- Jie Yu


On Nov. 24, 2017, 2:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63731/
> ---
> 
> (Updated Nov. 24, 2017, 2:08 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8207
> https://issues.apache.org/jira/browse/MESOS-8207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When resource providers update their state they send a list of
> pending or unacknowledged operations to the agent. This patch add
> tracking for such operations to the agent. The agent can then forward
> these operations to the master, e.g., for calculating the unused
> resources behind an agent.
> 
> We track an operation until we either receive a updated list of
> pending or unacknowledged operations from a resource provider, or
> until we see an acknowledgement from a framework. This keeps the list
> of operations bounded and ensures that we maintain the latest
> information in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 
> 
> 
> Diff: https://reviews.apache.org/r/63731/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`, tested as part of https://reviews.apache.org/r/63843/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 63830: Fixed 'NoOffersWithAllRolesSuppressed' test.

[Jiang Yan Xu]

> On Nov. 21, 2017, 9:53 a.m., Alexander Rukletsov wrote:
> > src/tests/scheduler_tests.cpp
> > Lines 1515-1516 (original), 1534-1535 (patched)
> > 
> >
> > `.WillRepeatedly(Return());`

There's only one agent with no tasks launched so I don't expect additional 
offers?


- Jiang Yan


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


On Nov. 21, 2017, 9:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63830/
> ---
> 
> (Updated Nov. 21, 2017, 9:46 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7996
> https://issues.apache.org/jira/browse/MESOS-7996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous iteration of the test failed to reveal the bug in MESOS-8200 due 
> to a race condition.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63830/diff/1/
> 
> 
> Testing
> ---
> 
> As expected, this test would reliably fail without /r/63741/ with the 
> following
> 
> ```
> I1115 07:48:28.536093 3117031 scheduler.cpp:741] Enqueuing event SUBSCRIBED 
> received from http://17.121.128.13:38423/master/api/v1/scheduler
> I1115 07:48:28.536224 3117031 scheduler.cpp:741] Enqueuing event HEARTBEAT 
> received from http://17.121.128.13:38423/master/api/v1/scheduler
> I1115 07:48:28.536244 3117029 master.cpp:8115] Sending 1 offers to framework 
> 560bd0e1-8ab7-4e7e-be7a-54cb5f399531- (default)
> I1115 07:48:28.536650 3117029 scheduler.cpp:741] Enqueuing event OFFERS 
> received from http://17.121.128.13:38423/master/api/v1/scheduler
> ../../src/tests/scheduler_tests.cpp:1497: Failure
> Mock function called more times than expected - returning directly.
> Function call: offers(0x7fffabd197a0, @0x7f7d8c005460 48-byte object 
> <50-98 0B-B0 7D-7F 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 
> 00-00 00-00 00-00 00-00 01-00 00-00 04-00 00-00 A0-54 00-8C 7D-7F 00-00>)
>  Expected: to be never called
>Actual: called once - over-saturated and active
> ```
> 
> The test passes with /r/63741/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 63862: Windows: Ported docker_tests.cpp.

[Akash Gupta]

> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 121-124 (patched)
> > 
> >
> > Would it be possible to instead change the default network setting to 
> > `bridge`? We may have talked about this, I don't remember. What should the 
> > default be if `host` doesn't work on Windows?
> 
> Akash Gupta wrote:
> I'm not sure. The default is set in the protobuf, so I'm not sure how the 
> default can be changed for Linux vs Windows. I guess we can have `host` AND 
> `bridge` = `nat`, but that seems hacky.

Resolved by having bridge and host = nat.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 399-401 (patched)
> > 
> >
> > Would this read better with an `all_of`?
> 
> Akash Gupta wrote:
> Yeah I can remove the loops with `std::algorithm`

eh, I think keeping the loop reads the best in this case.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 578-624 (original), 703-752 (patched)
> > 
> >
> > Wait, why did we remove it from the list of skipped tests?
> 
> Akash Gupta wrote:
> I #ifdef'd out the tests that won't work due to limitations in Windows at 
> the OS level based off this JIRA issue 
> https://issues.apache.org/jira/browse/MESOS-6392 . Should they still be 
> TEST_F_TEMP_DISABLED_ON_WINDOWS?

after discussing, we agreed to change to TEST_F_TEMP_DISABLED_ON_WINDOWS.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 790-919 (original), 926-1058 (patched)
> > 
> >
> > Why are these being compiled out?
> 
> Akash Gupta wrote:
> I #ifdef'd out the tests that won't work due to limitations in Windows at 
> the OS level based off this JIRA issue 
> https://issues.apache.org/jira/browse/MESOS-6392 . Should they still be 
> TEST_F_TEMP_DISABLED_ON_WINDOWS?

after discussing, we agreed to change to TEST_F_TEMP_DISABLED_ON_WINDOWS


- Akash


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


On Nov. 27, 2017, 5:45 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> ---
> 
> (Updated Nov. 27, 2017, 5:45 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ported the disabled tests to run on Windows. The following tests
> could not be ported due to Windows platform limitations and remain
> diabled:
>   - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
> sandbox).
>   - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
>   - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_tests.cpp 
> 5cabf5a0b0164f9923008677c58806c8931cbc8d 
> 
> 
> Diff: https://reviews.apache.org/r/63862/diff/2/
> 
> 
> Testing
> ---
> 
> Windows mesos-test:
> [==] 754 tests from 77 test cases ran. (270586 ms total)
> [  PASSED  ] 754 tests.
> 
> 
> Windows DockerTest only:
> [==] 11 tests from 1 test case ran. (85617 ms total)
> [  PASSED  ] 11 tests.
> 
> Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia 
> GPU):
> [==] 12 tests from 1 test case ran. (12413 ms total)
> [  PASSED  ] 12 tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63862: Windows: Ported docker_tests.cpp.

[Akash Gupta]

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

(Updated Nov. 27, 2017, 5:45 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Rebased + addressed feedback. I also changed the image to use the nanserver 
image instead since it is 50x smaller. However, the nanoserver image doesn't 
work properly with volume mounts in process isolation mode, so I changed the 
mount tests to use hyper-v isolation, which works fine.


Summary (updated)
-

Windows: Ported docker_tests.cpp.


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


Repository: mesos


Description (updated)
---

Ported the disabled tests to run on Windows. The following tests
could not be ported due to Windows platform limitations and remain
diabled:
  - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
sandbox).
  - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
  - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).


Diffs (updated)
-

  src/tests/containerizer/docker_tests.cpp 
5cabf5a0b0164f9923008677c58806c8931cbc8d 


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

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


Testing (updated)
---

Windows mesos-test:
[==] 754 tests from 77 test cases ran. (270586 ms total)
[  PASSED  ] 754 tests.


Windows DockerTest only:
[==] 11 tests from 1 test case ran. (85617 ms total)
[  PASSED  ] 11 tests.

Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia GPU):
[==] 12 tests from 1 test case ran. (12413 ms total)
[  PASSED  ] 12 tests.


Thanks,

Akash Gupta

Re: Review Request 63861: Windows: Updated networking doc.

[Akash Gupta]

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

(Updated Nov. 27, 2017, 5:38 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Rebased and changed host mode to nat mode on Windows


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


Repository: mesos


Description
---

The networking docs now describe how the Docker network modes in the
`Network` enum work on Windows, since the enum only has Linux network
modes.


Diffs (updated)
-

  docs/networking.md bdd3a762435aae163fc659ccfea7da825983 


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

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


Testing
---


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Nov. 27, 2017, 5:37 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Rebased and changed host mode to nat mode on Windows.


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


Repository: mesos


Description (updated)
---

The current Network enum in DockerInfo is specific to Linux containers.
Instead of supporting {host, bridge, none, user} networks, Windows
docker supports {nat, none, user} networks. Now, if the host or bridge
network type is sent to the Windows agent, it will be internally
converted to nat.


Diffs (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


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

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


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

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

(Updated Nov. 27, 2017, 5:36 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

rebased


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


Repository: mesos


Description
---

Also fixed the WEXITSTATUS macro to cast the exit code instead of
bit-masking it, since Windows exit codes are 32 bit unsigned ints.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


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

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


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63212: Added a findByTarget method for fs::MountInfoTable.

[Jie Yu]

> On Oct. 25, 2017, 9:26 p.m., James Peach wrote:
> > src/linux/fs.hpp
> > Lines 276 (patched)
> > 
> >
> > Not "immediate parent", but longest match right?

It's more complicated than that. I think the goal is to find the mount table 
entry that contains the given target (similar to `findmnt --target [TARGET]`).


- Jie


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


On Nov. 27, 2017, 4:49 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63212/
> ---
> 
> (Updated Nov. 27, 2017, 4:49 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7306
> https://issues.apache.org/jira/browse/MESOS-7306
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This method allows us to get the mount table entry that contains the
> specified target.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
>   src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
> 
> 
> Diff: https://reviews.apache.org/r/63212/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63999: Added env var to set default flags for parallel test runner.

[Armand Grillet]

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


Ship it!




Ship It!

- Armand Grillet


On nov. 27, 2017, 3:44 après-midi, Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63999/
> ---
> 
> (Updated nov. 27, 2017, 3:44 après-midi)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch modifies the parallel test runner to examine the
> environment variable 'MESOS_GTEST_RUNNER_FLAGS' for a default set of
> flags to pass. Flags given explicitly on the command line always have
> precedence over default flags from the environment variable.
> 
> This allows e.g., to set the default level of parallelism to not
> overload systems with many CPUs during 'make check' (which could
> ultimately lead to tests hitting timeouts).
> 
> 
> Diffs
> -
> 
>   support/mesos-gtest-runner.py 408e661f4fc9d0060c0eb7a45f1dfd1396f4f6c6 
> 
> 
> Diff: https://reviews.apache.org/r/63999/diff/2/
> 
> 
> Testing
> ---
> 
> Verified manually that
> 
> $ MESOS_GTEST_RUNNER_FLAGS=-j20 ../support/mesos-gtest-runner.py 
> ./3rdparty/stout/tests/stout-tests
> 
> executes 20 concurrent runs and that
> 
> $ MESOS_GTEST_RUNNER_FLAGS=-j20 ../support/mesos-gtest-runner.py -j6 
> ./3rdparty/stout/tests/stout-tests
> 
> executes 6 concurrent runs and that
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 63999: Added env var to set default flags for parallel test runner.

[Benjamin Bannier]

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

(Updated Nov. 27, 2017, 4:44 p.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Added some explanation of implementation details to comment.


Repository: mesos


Description
---

This patch modifies the parallel test runner to examine the
environment variable 'MESOS_GTEST_RUNNER_FLAGS' for a default set of
flags to pass. Flags given explicitly on the command line always have
precedence over default flags from the environment variable.

This allows e.g., to set the default level of parallelism to not
overload systems with many CPUs during 'make check' (which could
ultimately lead to tests hitting timeouts).


Diffs (updated)
-

  support/mesos-gtest-runner.py 408e661f4fc9d0060c0eb7a45f1dfd1396f4f6c6 


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

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


Testing
---

Verified manually that

$ MESOS_GTEST_RUNNER_FLAGS=-j20 ../support/mesos-gtest-runner.py 
./3rdparty/stout/tests/stout-tests

executes 20 concurrent runs and that

$ MESOS_GTEST_RUNNER_FLAGS=-j20 ../support/mesos-gtest-runner.py -j6 
./3rdparty/stout/tests/stout-tests

executes 6 concurrent runs and that


Thanks,

Benjamin Bannier

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Benjamin Bannier]

> On Nov. 26, 2017, 4:40 a.m., Jie Yu wrote:
> > src/master/master.hpp
> > Lines 2795-2841 (original)
> > 
> >
> > This should still be part of `addOfferOperation`. In fact, anything to 
> > do with `used` resourecs should still be part of `addOfferOperation`.
> > 
> > Only the part that's converting `total` resources should be pulled out 
> > because the way we change total does not rely on each offer operation. 
> > instead, we rely on the snapshot value received in `UpdateSlaveMessage`.
> 
> Benjamin Bannier wrote:
> While we do not strictly need to remove the updates to `used` here, I 
> feel it might be worthwhile to nevertheless completely remove any updates to 
> resource state here, and instead make this function operate exclusively on 
> offer operation state (and additionally in the future e.g., trigger offer 
> operation status reconciliation). To me this seems like a natural division of 
> responsibilities to me.

Discussed this offline with Jie. A strong argument in favor of still 
manipulating the used resources here is consistency with how tasks are 
currently handled.


- Benjamin


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


On Nov. 24, 2017, 12:24 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64054/
> ---
> 
> (Updated Nov. 24, 2017, 12:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applying operations has been refactored out of 'addOfferOperation' and
> simplified by using 'protobuf::isSpeculativeOperation'.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
>   src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
>   src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 
> 
> 
> Diff: https://reviews.apache.org/r/64054/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63999: Added env var to set default flags for parallel test runner.

[Armand Grillet]

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




support/mesos-gtest-runner.py
Lines 136 (patched)


Could you please add a comment explaining how does the flags passed on the 
command line have precedence over the defaults?


- Armand Grillet


On Nov. 21, 2017, 9:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63999/
> ---
> 
> (Updated Nov. 21, 2017, 9:09 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch modifies the parallel test runner to examine the
> environment variable 'MESOS_GTEST_RUNNER_FLAGS' for a default set of
> flags to pass. Flags given explicitly on the command line always have
> precedence over default flags from the environment variable.
> 
> This allows e.g., to set the default level of parallelism to not
> overload systems with many CPUs during 'make check' (which could
> ultimately lead to tests hitting timeouts).
> 
> 
> Diffs
> -
> 
>   support/mesos-gtest-runner.py 408e661f4fc9d0060c0eb7a45f1dfd1396f4f6c6 
> 
> 
> Diff: https://reviews.apache.org/r/63999/diff/1/
> 
> 
> Testing
> ---
> 
> Verified manually that
> 
> $ MESOS_GTEST_RUNNER_FLAGS=-j20 ../support/mesos-gtest-runner.py 
> ./3rdparty/stout/tests/stout-tests
> 
> executes 20 concurrent runs and that
> 
> $ MESOS_GTEST_RUNNER_FLAGS=-j20 ../support/mesos-gtest-runner.py -j6 
> ./3rdparty/stout/tests/stout-tests
> 
> executes 6 concurrent runs and that
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64081: Fixed a code snippet in libprocess documentation.

[Alexander Rukletsov]

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

(Updated Nov. 27, 2017, 3:27 p.m.)


Review request for mesos and Benjamin Hindman.


Summary (updated)
-

Fixed a code snippet in libprocess documentation.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/README.md ba09653dbc26a997856b712c9f082c6e06c39137 


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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 64081: Fix a code snippet in libprocess documentation.

[Benjamin Hindman]

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


Ship it!




Ship It!

- Benjamin Hindman


On Nov. 27, 2017, 11:06 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64081/
> ---
> 
> (Updated Nov. 27, 2017, 11:06 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md ba09653dbc26a997856b712c9f082c6e06c39137 
> 
> 
> Diff: https://reviews.apache.org/r/64081/diff/1/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64086: Removed currently unneeded 'AWAIT_READY's in 'MockResourceProvider'.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On Nov. 27, 2017, 4:18 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64086/
> ---
> 
> (Updated Nov. 27, 2017, 4:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'AWAIT_READY's in 'MockResourceProvider' let to the resource
> provider not being usable with paused clock as we would run into a
> deadlock in that case.
> 
> This patch removes the explicit awaits here to make the resource
> provider usable. We should consider making it easier to debug send
> failures in the resource provider, e.g., by surfacing the future
> responses to users in some way, or by added additional logging.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp e5c2b697a4401f010a7d77ae9acb8a9cbd3846bc 
> 
> 
> Diff: https://reviews.apache.org/r/64086/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`, tests as part of https://reviews.apache.org/r/63843/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64086: Removed currently unneeded 'AWAIT_READY's in 'MockResourceProvider'.

[Benjamin Bannier]

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

(Updated Nov. 27, 2017, 4:18 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Repository: mesos


Description (updated)
---

The 'AWAIT_READY's in 'MockResourceProvider' let to the resource
provider not being usable with paused clock as we would run into a
deadlock in that case.

This patch removes the explicit awaits here to make the resource
provider usable. We should consider making it easier to debug send
failures in the resource provider, e.g., by surfacing the future
responses to users in some way, or by added additional logging.


Diffs
-

  src/tests/mesos.hpp e5c2b697a4401f010a7d77ae9acb8a9cbd3846bc 


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


Testing
---

`make check`, tests as part of https://reviews.apache.org/r/63843/.


Thanks,

Benjamin Bannier

Re: Review Request 63953: Added logging based on container class.

[Alexander Rukletsov]

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




src/logging/utils.hpp
Lines 17-18 (patched)


Can you please remind me why you create a separate file instead of adding 
it to "mesos/src/logging/logging.hpp"?



src/logging/utils.hpp
Lines 28-35 (patched)


Maybe this capture the intention better?
```
#define LOG_BASED_ON_CLASS(containerClass) \
  LOG_IF(INFO, (containerClass != ContainerClass::DEBUG) || VLOG_IS_ON(1))
```



src/slave/containerizer/mesos/containerizer.cpp
Lines 1928-1929 (original), 1932-1933 (patched)


What about this?



src/slave/containerizer/mesos/containerizer.cpp
Lines 2322-2323 (original), 2326-2329 (patched)


Let's add a utility function into `Container` struct. With a todo saying 
this function will go away once `config` is not optional.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2328 (patched)


Option supports `operator->`



src/slave/containerizer/mesos/containerizer.cpp
Lines 2792-2795 (original), 2815-2818 (patched)


What about this?


- Alexander Rukletsov


On Nov. 27, 2017, 2:16 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63953/
> ---
> 
> (Updated Nov. 27, 2017, 2:16 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7361
> https://issues.apache.org/jira/browse/MESOS-7361
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adjusts log level based on the container class.
> If the class is `DEBUG` we print the log entry at a verbose
> level 1, otherwise we print it at the `INFO` level.
> 
> We use the added macro in mesos containerizer so that COMMAND
> checks cause less INFO logs (15 lines instead of 26 before).
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
>   src/logging/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29 
> 
> 
> Diff: https://reviews.apache.org/r/63953/diff/5/
> 
> 
> Testing
> ---
> 
> Started a Mesos cluster and used `mesos-execute` with this task group to 
> check that the behaviour after this patch is the one expected:
> 
> ```
> {
>   "tasks": [
> {
>   "name": "Name of the task",
>   "task_id": {
> "value": "task-group"
>   },
>   "agent_id": {
> "value": ""
>   },
>   "resources": [
> {
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
> "value": 0.01
>   }
> },
> {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
> "value": 2
>   }
> }
>   ],
>   "command": {
> "value": "sleep 1000"
>   },
>   "check": {
> "type": "COMMAND",
> "command": {
>   "command": {
> "value": "echo \"Bonjour\""
>   },
>   "uris": []
> }
>   }
> }
>   ]
> }
> ```
> 
> And:
> ```
> $ nice make check
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 63842: Allowed removing non-terminal offer operations.

[Benjamin Bannier]

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

(Updated Nov. 27, 2017, 4 p.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.


Repository: mesos


Description
---

During reconcilation we might be required to remove non-terminal offer
operations from bookkeeping.


Diffs
-

  src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
  src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
  src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 


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


Testing
---

`make check`, tested as part of https://reviews.apache.org/r/63843/.

This patch requires `protobuf::isSpeculativeOperation` from 
https://reviews.apache.org/r/63751/ which is _not_ part of this review chain 
(to avoid multiple dependent RRs).


Thanks,

Benjamin Bannier

Re: Review Request 64000: Allowed stringification of hashmaps of stringifiable types.

[Benjamin Bannier]

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

(Updated Nov. 27, 2017, 4 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Repository: mesos


Description
---

Allowed stringification of hashmaps of stringifiable types.


Diffs
-

  include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
  src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 63997: Added a new allocator method to add resources to agents.

[Benjamin Bannier]

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

(Updated Nov. 27, 2017, 3:55 p.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Changes
---

Rebased.


Repository: mesos


Description
---

The added method complements 'Allocator::addSlave'. While in
'addSlave' the total agent resources and used resources are passed,
the method 'addResourceProvider' added here allows to add additional,
potentially used resources to an existing agent.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
ae122003487ca8956573e993cd3993aa8cc286f1 
  src/master/allocator/mesos/allocator.hpp 
8fa4fdeec4ec64bcd49fc442c230d8684a11cfd9 
  src/master/allocator/mesos/hierarchical.hpp 
2c4832b29842330fa57756cd3d4202f265a820f3 
  src/master/allocator/mesos/hierarchical.cpp 
5ce9ceaa3a5f84a1e076d45448863c418531cc2b 
  src/tests/allocator.hpp 6a84f1beb86dceb5a5e9bf4615c13a216f3d0905 
  src/tests/hierarchical_allocator_tests.cpp 
f0f95ba4f667bf8ea54e985d8cde913a4170d8ff 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 63843: Implemented a test of offer operation reconcilation.

[Benjamin Bannier]

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

(Updated Nov. 27, 2017, 3:55 p.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.


Changes
---

Simplified test by using default actions of `MockResourceProvider`.


Repository: mesos


Description
---

Whenever a speculated operation fails in a resource provider, we
expect the agent to trigger a 'UpdateSlaveMessage' to the master so it
can rollback its agent state. This patch adds such a test.


Diffs (updated)
-

  src/tests/slave_tests.cpp a2274b691cf94b003c4bc15450d176a9c73517d5 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 64086: Removed currently unneeded 'AWAIT_READY's in 'MockResourceProvider'.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


Repository: mesos


Description
---

The 'AWAIT_READY's in 'MockResourceProvider' let to the resource
provider not being usable with paused clock as we would run into a
deadclock in that case.

This patch removes the explicit awaits here to make this resource
provider usable. We should consider making it easier to debug send
failures in the resource provider, e.g., by surfacing the future
responses to users in some way, or by added additional logging.


Diffs
-

  src/tests/mesos.hpp e5c2b697a4401f010a7d77ae9acb8a9cbd3846bc 


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


Testing
---

`make check`, tests as part of https://reviews.apache.org/r/63843/.


Thanks,

Benjamin Bannier

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

(Updated Nov. 27, 2017, 10:43 p.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addessed the issue on macOS that the `connect()` function will return the error 
`Invalid argument`.
The root cause of this issue is, on macOS setting the 3rd paramter (`addrlen`) 
of the `connect()` function to the size of `sockaddr_storage` is not correct, 
instead we should set it to `sockaddr_in` (IPv4) or `sockaddr_in6` (IPv6).


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs (updated)
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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

Changes: https://reviews.apache.org/r/63795/diff/5-6/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 63953: Added logging based on container class.

[Armand Grillet]

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

(Updated Nov. 27, 2017, 2:16 p.m.)


Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description (updated)
---

This change adjusts log level based on the container class.
If the class is `DEBUG` we print the log entry at a verbose
level 1, otherwise we print it at the `INFO` level.

We use the added macro in mesos containerizer so that COMMAND
checks cause less INFO logs (15 lines instead of 26 before).


Diffs
-

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/logging/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/containerizer.cpp 
7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29 


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


Testing (updated)
---

Started a Mesos cluster and used `mesos-execute` with this task group to check 
that the behaviour after this patch is the one expected:

```
{
  "tasks": [
{
  "name": "Name of the task",
  "task_id": {
"value": "task-group"
  },
  "agent_id": {
"value": ""
  },
  "resources": [
{
  "name": "cpus",
  "type": "SCALAR",
  "scalar": {
"value": 0.01
  }
},
{
  "name": "mem",
  "type": "SCALAR",
  "scalar": {
"value": 2
  }
}
  ],
  "command": {
"value": "sleep 1000"
  },
  "check": {
"type": "COMMAND",
"command": {
  "command": {
"value": "echo \"Bonjour\""
  },
  "uris": []
}
  }
}
  ]
}
```

And:
```
$ nice make check
```


Thanks,

Armand Grillet