Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Guangya Liu]

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


Fix it, then Ship it!




Ship It!


src/common/resources.cpp (lines 1274 - 1276)


This indicates that if the `Resource` do not have `AllocationInfo`, then we 
will loop all of the `Resource` till finished then return false, but we have 
the assumption of no `Resources` store a mix of allocated and unallocated 
resources, so how about `return resource.has_allocation_info();` here?



src/tests/resources_utils.hpp (line 27)


I think that we do not like `using xxx` in a header file.



src/tests/resources_utils.hpp (line 33)


I think that we can kill the keyword `TODO` here?


- Guangya Liu


On 一月 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated 一月 23, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56016: Expanded a comment about TaskStatus protobuf.

[Vinod Kone]

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




include/mesos/mesos.proto (lines 1768 - 1771)


I think this comment belongs in `Update` proto in scheduler.proto and 
`StatusUpdate` proto, not here.


- Vinod Kone


On Jan. 27, 2017, 12:53 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56016/
> ---
> 
> (Updated Jan. 27, 2017, 12:53 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 37a97f6bb3c229c0ae16a263e4de458010fba32b 
>   include/mesos/v1/mesos.proto 31563b79010fff4c07b1c0ab691c0c7635035845 
> 
> Diff: https://reviews.apache.org/r/56016/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55884: Updated the naming of unacknowledged tasks in executors for clarity.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 27, 2017, 12:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55884/
> ---
> 
> (Updated Jan. 27, 2017, 12:10 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
> 
> Diff: https://reviews.apache.org/r/55884/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55761: Fixed name matching for automatic resources.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 25, 2017, 7:10 a.m., Bruce Merry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55761/
> ---
> 
> (Updated Jan. 25, 2017, 7:10 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6821
> https://issues.apache.org/jira/browse/MESOS-6821
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of doing a simple substring search for known resources like
> "cpus" to decide whether they have been explicitly specified, it now
> parses the resource string to check that a resource with that exact
> name was specified.
> 
> This splits out a fromString static member function from the start of
> Resources::parse that does parsing using either fromJSON or
> fromSimpleString and makes it available to use in
> mesos::internal::slave::Containerizer::resources.
> 
> Fixes MESOS-6821.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   src/Makefile.am 3d0e82cb522d379923e00e5e98e9c9f646fdd3db 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/containerizer/containerizer_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55761/diff/
> 
> 
> Testing
> ---
> 
> Regression test added.
> 
> 
> Thanks,
> 
> Bruce Merry
> 
>

Re: Review Request 55901: [WIP] Added support for command health checks to the default executor.

[Vinod Kone]

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




src/checks/health_checker.hpp (line 137)


we don't typically do `const` for POD types.

s/_agentSpawnsCommandContainer/



src/checks/health_checker.cpp (line 129)


s/build/create/



src/checks/health_checker.cpp (lines 145 - 157)


can you inline these?



src/checks/health_checker.cpp (lines 426 - 429)


why a `.repair()` here?



src/checks/health_checker.cpp (line 435)


const & ?



src/checks/health_checker.cpp (line 447)


s/this/This/



src/checks/health_checker.cpp (line 449)


s/Agent/agent/



src/checks/health_checker.cpp (line 480)


launch nested container session returns a streaming response, how come you 
are calling `post()` helper here which expects a non-streaming response?

probably one of the reasons why your test is hanging.



src/checks/health_checker.cpp (lines 481 - 485)


The identation seems wrong? do we do it like this elsehwhere?

i would've done so

```
.after(checkTimeout,
   defer(self(),
 ...)
```



src/checks/health_checker.cpp (line 531)


s/has not returned/timed out/



src/checks/health_checker.cpp (lines 534 - 536)


why repair?



src/checks/health_checker.cpp (line 538)


why not just return failure?



src/checks/health_checker.cpp (line 562)


s/callResponse/response/



src/launcher/default_executor.cpp (line 112)


s/resume/Resume/



src/launcher/default_executor.cpp (line 130)


s/stop/Stop/


- Vinod Kone


On Jan. 25, 2017, 12:33 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Jan. 25, 2017, 12:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [WIP] Added support for command health checks to the default executor.
> 
> This is still a work in progress, but I am posting it in order to get some 
> feedback on the general approach.
> 
> Open questions/issues:
> 
>   - The code that handles communication with the Agent is on the 
> `DefaultExecutor`, so the health checker calls `evolve` when serializing the 
> API calls. Should it use the v1 protos instead, and skip the `evolve` call?
>   - CMD health checks for command tasks inherit the env from the executor, 
> but allow overriding env variables. I don't think that inheriting the 
> executor env makes sense for cmd health checks launched by the 
> `DefaultExecutor` - each task can have its own env, so the health check 
> command should inherit the task env and merge it with the one in the check's 
> `CommandInfo`.
> I added some code that takes the task env from `TaskInfo` and merges it 
> check's env, but I don't think that this is an ideal approach, since it will 
> miss env variables set/modified by containerizers/hooks/isolators. Do you 
> think that it would make sense for all Debug Containers to inherit the 
> environment of the parent container?
>   - There's a TODO for stopping/resuming health checking when the 
> `DefaultExecutor` is disconnected from the agent. After talking to AlexR, I 
> believe that we should do this for all types of checks, not just for `CMD`.
>   - The test that I introduced passes on Linux, but not on macOS, I still 
> have to do some more debugging.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 6e558f2061a9e31157c47d31cb64b3a8568aace3 
>   src/checks/health_checker.cpp 50aa2858e807b27bbab58a3618f5200cfe4eca9e 
>   src/launcher/default_executor.cpp a03794934adb93868734f8cf00b337a1bff9b5ab 
>   src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> 

Re: Review Request 55694: CMake: Separated Stout system headers from Stout API headers.

[Joseph Wu]

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


Ship it!




LGTM.

- Joseph Wu


On Jan. 18, 2017, 5:23 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55694/
> ---
> 
> (Updated Jan. 18, 2017, 5:23 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-3932
> https://issues.apache.org/jira/browse/MESOS-3932
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3932 specifies a bug that causes spurious warnings to be printed
> when building Mesos.
> 
> Many of these warnings can be eliminated simply by correctly marking off
> the third-party dependencies as `SYSTEM` headers when we specify the
> include path.
> 
> This commit will thus separate out the headers of Stout's third-party
> dependencies from the core Stout API headers, and include them as
> `SYSTEM` headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> bc27ac687bae4e1798eece562027ba33c6b32348 
>   3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> d3bd72e8eba77213095da6cabb3a6d6f4d30941c 
>   3rdparty/stout/tests/CMakeLists.txt 
> a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae 
> 
> Diff: https://reviews.apache.org/r/55694/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55695: CMake: Separated Libprocess system headers from Libprocess API headers.

[Joseph Wu]

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


Ship it!




LGTM.

- Joseph Wu


On Jan. 18, 2017, 5:23 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55695/
> ---
> 
> (Updated Jan. 18, 2017, 5:23 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-3932
> https://issues.apache.org/jira/browse/MESOS-3932
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3932 specifies a bug that causes spurious warnings to be printed
> when building Mesos.
> 
> Many of these warnings can be eliminated simply by correctly marking off
> the third-party dependencies as `SYSTEM` headers when we specify the
> include path.
> 
> This commit will thus separate out the headers of Libprocesses's
> third-party dependencies from the core Libprocess API headers, and
> include them as `SYSTEM` headers.
> 
> Notably, we choose to include Stout's headers as third-party system
> headers, since the goal is for them to be completely independent
> projects.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 873f41d844faa0d442f77411e94314a89be5f046 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 49ad836d5fa3f84cdf5ae0e08f449cd7ef2537a1 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 60f0e76dfd237d9a12a366b413802d1a96892b55 
> 
> Diff: https://reviews.apache.org/r/55695/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56037: CMake: Bumped CMake version on Windows, and enforce with check.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 27, 2017, 12:45 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56037/
> ---
> 
> (Updated Jan. 27, 2017, 12:45 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will bump the required CMake version on Windows from ~3.5 to
> 3.6.3, and then add a configure-time check to ensure that this is true
> before we build.
> 
> On Windows, bumping the required version is much less of a big deal than
> Unix, mainly because Windows does not have a package manager. The burden
> on developers is about the same, independent of the version, because the
> most convenient installation option is to use a `.msi`. Additionally,
> CMake is not widely used in the community, and where it is, the vast
> majority of users are building on Unix, so there are few constraints
> imposed by existing CI systems and users.
> 
> The primary reason to bump the version, though, is that the CMake
> featureset on Windows tends (consciously or not) to straggle a bit
> behind the Unix featureset, and recently it has come to our attention
> that (due to our ignorance of this issue) we have come to depend on some
> subtle features that are available on Unix, but not on Windows, until
> CMake 3.6.3.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f 
>   docs/windows.md 5a5f934180f11fd6260032551f0df65fde541218 
> 
> Diff: https://reviews.apache.org/r/56037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55748: CMake: Deleted spurious configuration settings in agent and master.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 19, 2017, 11:31 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55748/
> ---
> 
> (Updated Jan. 19, 2017, 11:31 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Deleted spurious configuration settings in agent and master.
> 
> 
> Diffs
> -
> 
>   src/master/cmake/MasterConfigure.cmake 
> 3d316d6ff2910fc360b0faecb5e6ac9687a77883 
>   src/slave/cmake/AgentConfigure.cmake 
> 1582127ccce773af6031a5b09252192b05a13cdc 
> 
> Diff: https://reviews.apache.org/r/55748/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55696: CMake: Separated Mesos system headers from Mesos API headers.

[Joseph Wu]

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




src/CMakeLists.txt (line 361)


I assume you meant to add a `SYSTEM` here?



src/docker/CMakeLists.txt (line 28)


And here.



src/slave/CMakeLists.txt (line 31)


And here.



src/tests/CMakeLists.txt (line 226)


And here.


- Joseph Wu


On Jan. 18, 2017, 5:23 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55696/
> ---
> 
> (Updated Jan. 18, 2017, 5:23 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-3932
> https://issues.apache.org/jira/browse/MESOS-3932
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3932 specifies a bug that causes spurious warnings to be printed
> when building Mesos.
> 
> Many of these warnings can be eliminated simply by correctly marking off
> the third-party dependencies as `SYSTEM` headers when we specify the
> include path.
> 
> This commit will thus separate out the headers of Mesos's
> third-party dependencies from the core Mesos API headers, and
> include them as `SYSTEM` headers.
> 
> Notably, we choose to include Stout's and Libprocess's headers as
> third-party system headers, since the goal is for them to be completely
> independent projects.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c 
>   src/docker/CMakeLists.txt 892370d603d42b649afbf05deece5e0b1d97ee98 
>   src/master/CMakeLists.txt a9fb34fc7099f05e279ee0506dd18bc0745d546c 
>   src/master/cmake/MasterConfigure.cmake 
> 3d316d6ff2910fc360b0faecb5e6ac9687a77883 
>   src/slave/CMakeLists.txt 03466bda934290ce41fa6408d35ccae10c9a6f32 
>   src/slave/cmake/AgentConfigure.cmake 
> 1582127ccce773af6031a5b09252192b05a13cdc 
>   src/tests/CMakeLists.txt b7adf4f3126611a57baf4f62b62b208bc0236da5 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> 8d416388a8e45a2832ae3841b58541ba5b0613bc 
> 
> Diff: https://reviews.apache.org/r/55696/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55637: CMake: Added `test` target.

[Joseph Wu]

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


Ship it!




LGTM, except your review description is out of date.

I'll replace part of it with:
```
This commit will introduce a "tests" target to the CMake build system.
The semantics of this target are very similar to the autotools build
target "test" (notice the extra "s" in the CMake target), in that
they build the tests, but do not run them.

We use "tests" in CMake because CMake expects to control the "test"
target itself.
```

- Joseph Wu


On Jan. 18, 2017, 3:34 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55637/
> ---
> 
> (Updated Jan. 18, 2017, 3:34 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-3697
> https://issues.apache.org/jira/browse/MESOS-3697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will introduce a `test` target to the CMake build system.
> The semantics of this target are very similar to the autotools build
> system, in that they build the tests, but do not run them.
> 
> Accomplishing this is somewhat complex in CMake, because CMake expects
> to control the `test` target itself. We work around this by (1)
> silencing the warning that `test` is a reserved target, (2) removing the
> call to `enable_testing` that sets up CTest, and (3) adding our own
> `test` target. As an additional insurance policy, we error out if any of
> the `BUILD_TESTING` flags are defined, which would indicate that
> `include(CTest)` was called.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f 
> 
> Diff: https://reviews.apache.org/r/55637/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55607: CMake: Added configuration of test scripts in the bin/ directory.

[Joseph Wu]

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


Ship it!




Quickly verified:
```
cmake ..
ls bin  # Looks correct

cat bin/mesos-agent.sh  # Looks correct
```


cmake/MesosConfigure.cmake (lines 222 - 224)


We generally put `NOTE`s on a newline.  And sometimes we put an extra 
newline before one (especially after big comment blocks).


- Joseph Wu


On Jan. 26, 2017, 1:20 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55607/
> ---
> 
> (Updated Jan. 26, 2017, 1:20 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6757
> https://issues.apache.org/jira/browse/MESOS-6757
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-6757.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake ce127e1e8c5c33ad8badef6fb3ac9f50ade9056f 
> 
> Diff: https://reviews.apache.org/r/55607/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55900: Improved style in `HealthChecker`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 25, 2017, 6:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55900/
> ---
> 
> (Updated Jan. 25, 2017, 6:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked `_taskPid` in the constructor as const, to be consistent with the
> other parameters.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 6e558f2061a9e31157c47d31cb64b3a8568aace3 
>   src/checks/health_checker.cpp 50aa2858e807b27bbab58a3618f5200cfe4eca9e 
> 
> Diff: https://reviews.apache.org/r/55900/diff/
> 
> 
> Testing
> ---
> 
> `make check` (macOS and Linux)
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55955: Added validation tests to ensure environment variable value is set.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 26, 2017, 2:36 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55955/
> ---
> 
> (Updated Jan. 26, 2017, 2:36 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4 
>   src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269 
>   src/tests/master_validation_tests.cpp 
> edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/55955/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --gtest_filter="*Validation*"
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 55954: Changed 'Environment.Variable.Value' from required to optional.

[Vinod Kone]

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




src/slave/validation.cpp (line 175)


put this on the above line? does it not fit?



src/slave/validation.cpp (line 252)


s/launch_nested_container()/launch_nested_container_session()/



src/slave/validation.cpp (line 254)


put this on the above line.



src/slave/validation.cpp (line 255)


ditto. session*


- Vinod Kone


On Jan. 26, 2017, 2:36 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55954/
> ---
> 
> (Updated Jan. 26, 2017, 2:36 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To prepare for future work which will enable the
> modular fetching of secrets, we should change the
> Environment.Variable.Value field from required to
> optional. This way, the field can be left empty
> and filled in by a secret fetching module.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 044b7e697f77c9e2cefb938bda4307d328b33766 
>   include/mesos/v1/mesos.proto 7706de89d91926aedb10e4c882dffcf050f03bb1 
>   src/checks/checker.cpp 8d7285af40d9633608178ec239d3b8d65d3a2725 
>   src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 
>   src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/master/validation.cpp e3f71be16ff21a1b4e10ad59846d9b06691bc1b9 
>   src/slave/validation.cpp 3fd32feb3cd0e6d30a66a917e20fd9ca50b84dc2 
> 
> Diff: https://reviews.apache.org/r/55954/diff/
> 
> 
> Testing
> ---
> 
> Testing information at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 55940: Added support for the new streaming request/response headers.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 26, 2017, 5:03 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55940/
> ---
> 
> (Updated Jan. 26, 2017, 5:03 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6936
> https://issues.apache.org/jira/browse/MESOS-6936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the Agent API/Switchboard handlers to support the
> "Message-Accept"/"Message-Content-Type" headers for request/response
> streaming. Also, adds general validations to ensure that non-streaming
> requests/responses don't have these headers set.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 1b8f490872b8a13dc7bc64883ed28080752f82b6 
>   src/slave/http.cpp 85990aee42195f8d2e0affa06c0bb5724f229247 
>   src/slave/slave.hpp 0dadbe50be15f89b791da55fa10f1b434693ee0f 
> 
> Diff: https://reviews.apache.org/r/55940/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 55978: Added some assertions to the switchboard request handler.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 26, 2017, 5:03 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55978/
> ---
> 
> (Updated Jan. 26, 2017, 5:03 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6936
> https://issues.apache.org/jira/browse/MESOS-6936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some of the request parameters e.g., request method/header values
> are already validated by the agent. We can replace them with explicit
> assertions in the IO switchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 1b8f490872b8a13dc7bc64883ed28080752f82b6 
> 
> Diff: https://reviews.apache.org/r/55978/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 55937: Added constants for referring to request/response streaming headers.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 26, 2017, 5:04 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55937/
> ---
> 
> (Updated Jan. 26, 2017, 5:04 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6936
> https://issues.apache.org/jira/browse/MESOS-6936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed the previous streaming headers and added constants for
> "Message-Accept and "Message-Content-Type" headers allowing us
> to refer to them later in the agent API handlers. Also, modified
> the stream operators as well as the serialize/deserialize
> functions to handle the new content-type.
> 
> 
> Diffs
> -
> 
>   include/mesos/http.hpp fdc4839615159adbc8af61dca7bfdfe5cd69a8b4 
>   src/common/http.hpp fc2ac148744bb819944044540fe7274bac0d60ba 
>   src/common/http.cpp c65c79e3f689b32fb9e62062fbc297b10501dcfe 
> 
> Diff: https://reviews.apache.org/r/55937/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56037: CMake: Bumped CMake version on Windows, and enforce with check.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [56037, 55749, 55748, 55696, 55695, 55694, 55637, 55632, 
55607, 55604, 55602, 55601, 55600, 55599, 0, 55549, 55547, 55546, 55544, 
55543, 55328, 55327, 55314, 55313, 55312, 55311, 55162, 55161, 55040, 55039, 
55038, 55037, 55699, 55030, 55024, 55023, 55022]

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

Error:
2017-01-27 21:26:20 URL:https://reviews.apache.org/r/55601/diff/raw/ 
[23809/23809] -> "55601.patch" [1]
error: patch failed: src/CMakeLists.txt:268
error: src/CMakeLists.txt: patch does not apply

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

- Mesos Reviewbot


On Jan. 27, 2017, 8:45 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56037/
> ---
> 
> (Updated Jan. 27, 2017, 8:45 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will bump the required CMake version on Windows from ~3.5 to
> 3.6.3, and then add a configure-time check to ensure that this is true
> before we build.
> 
> On Windows, bumping the required version is much less of a big deal than
> Unix, mainly because Windows does not have a package manager. The burden
> on developers is about the same, independent of the version, because the
> most convenient installation option is to use a `.msi`. Additionally,
> CMake is not widely used in the community, and where it is, the vast
> majority of users are building on Unix, so there are few constraints
> imposed by existing CI systems and users.
> 
> The primary reason to bump the version, though, is that the CMake
> featureset on Windows tends (consciously or not) to straggle a bit
> behind the Unix featureset, and recently it has come to our attention
> that (due to our ignorance of this issue) we have come to depend on some
> subtle features that are available on Unix, but not on Windows, until
> CMake 3.6.3.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f 
>   docs/windows.md 5a5f934180f11fd6260032551f0df65fde541218 
> 
> Diff: https://reviews.apache.org/r/56037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 56037: CMake: Bumped CMake version on Windows, and enforce with check.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Repository: mesos


Description
---

This commit will bump the required CMake version on Windows from ~3.5 to
3.6.3, and then add a configure-time check to ensure that this is true
before we build.

On Windows, bumping the required version is much less of a big deal than
Unix, mainly because Windows does not have a package manager. The burden
on developers is about the same, independent of the version, because the
most convenient installation option is to use a `.msi`. Additionally,
CMake is not widely used in the community, and where it is, the vast
majority of users are building on Unix, so there are few constraints
imposed by existing CI systems and users.

The primary reason to bump the version, though, is that the CMake
featureset on Windows tends (consciously or not) to straggle a bit
behind the Unix featureset, and recently it has come to our attention
that (due to our ignorance of this issue) we have come to depend on some
subtle features that are available on Unix, but not on Windows, until
CMake 3.6.3.


Diffs
-

  CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f 
  docs/windows.md 5a5f934180f11fd6260032551f0df65fde541218 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 54215: Fixed duplicate image layer ids returned by docker store.

[Gilbert Song]

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

(Updated Jan. 27, 2017, 11:46 a.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian 
Zhang, and Zhitao Li.


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


Repository: mesos


Description
---

This issue is exposed by pulling the 'mesosphere/inky' docker
image using registry puller. Due to the duplicate layer id
from the manifest, there are duplicate layer pathes passed
to the backend. The aufs backend cannot handle this case and
returns 'invalid arguments' error. Ideally, we should make
sure that layer paths that are passed to the backend are
unique.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
cecf34a23329a64fdbce7de4b83827a30975e9a4 

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


Testing
---

make check

Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.

Manually tested using the `mesosphere/inky` image, which contains duplicate 
layer ids.


Thanks,

Gilbert Song

Re: Review Request 54215: Fixed duplicate image layer ids returned by docker store.

[Gilbert Song]

> On Jan. 27, 2017, 8:43 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp, line 
> > 301
> > 
> >
> > I'd add some comment about the order of `fslayers` here. whether it is:
> > ```
> > [parent, child]
> > ```
> > or
> > ```
> > [child, parent]
> > ```
> > 
> > Since child layer can overwrite contents in the parent layer, I think 
> > what backend expects is `[parent, child]`.
> > 
> > That means fslayers here is in `[child, parent]` order. Now, can you 
> > verify that the overwriting is ok. I'd improve the comment here.

You are correct. It can be proved in any docker image manifest.

```
   "history": [
  {
 "v1Compatibility": 
"{\"architecture\":\"amd64\",\"config\":{\"Hostname\":\"26ba10d264c2\",\"Domainname\":\"\",\"User\":\"\",\"AttachStdin\":false,\"AttachStdout\":false,\"AttachStderr\":false,\"ExposedPorts\":{\"5000/tcp\":{}},\"Tty\":false,\"OpenStdin\":false,\"StdinOnce\":false,\"Env\":[\"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\"],\"Cmd\":[\"/etc/docker/registry/config.yml\"],\"Image\":\"sha256:ab9c83929fc3997f57912fb54a5dbaeff61a506b23d56b91591e665082f2ec9e\",\"Volumes\":{\"/var/lib/registry\":{}},\"WorkingDir\":\"\",\"Entrypoint\":[\"/entrypoint.sh\"],\"OnBuild\":[],\"Labels\":{}},\"container\":\"876f643aaa16e30f99d14d6fd707432c8c8606696a18c53303d382761fb6573f\",\"container_config\":{\"Hostname\":\"26ba10d264c2\",\"Domainname\":\"\",\"User\":\"\",\"AttachStdin\":false,\"AttachStdout\":false,\"AttachStderr\":false,\"ExposedPorts\":{\"5000/tcp\":{}},\"Tty\":false,\"OpenStdin\":false,\"StdinOnce\":false,\"Env\":[\"PATH=/usr/local/sbin:/usr/local/bi
 n:/usr/sbin:/usr/bin:/sbin:/bin\"],\"Cmd\":[\"/bin/sh\",\"-c\",\"#(nop) 
\",\"CMD 
[\\"/etc/docker/registry/config.yml\\"]\"],\"Image\":\"sha256:ab9c83929fc3997f57912fb54a5dbaeff61a506b23d56b91591e665082f2ec9e\",\"Volumes\":{\"/var/lib/registry\":{}},\"WorkingDir\":\"\",\"Entrypoint\":[\"/entrypoint.sh\"],\"OnBuild\":[],\"Labels\":{}},\"created\":\"2017-01-18T05:14:00.691185561Z\",\"docker_version\":\"1.12.6\",\"id\":\"46afc3aaeab95b4c8226da6a3a65a7c24dd012a9c756e8134e136c47333f8271\",\"os\":\"linux\",\"parent\":\"a375b425d3042b81f88b2cea92aecc1ce4e621bcdbfcd96807db2f9878687e76\",\"throwaway\":true}"
  },
  {
 "v1Compatibility": 
"{\"id\":\"a375b425d3042b81f88b2cea92aecc1ce4e621bcdbfcd96807db2f9878687e76\",\"parent\":\"ca1c9bbf547c023906271d9a4cc8b5a2c006dada5aecf221768c08ed052f2b28\",\"created\":\"2017-01-18T05:14:00.170761825Z\",\"container_config\":{\"Cmd\":[\"/bin/sh
 -c #(nop)  ENTRYPOINT [\\"/entrypoint.sh\\"]\"]},\"throwaway\":true}"
  },
  {
 "v1Compatibility": 
"{\"id\":\"ca1c9bbf547c023906271d9a4cc8b5a2c006dada5aecf221768c08ed052f2b28\",\"parent\":\"262f434a6db5dfc7e5088923b4eed39b6e837f160a403de168d255782896ca8c\",\"created\":\"2017-01-18T05:13:59.523763783Z\",\"container_config\":{\"Cmd\":[\"/bin/sh
 -c #(nop) COPY 
file:7b57f7ab1a8cf85c00768560fffc926543a60c9c9f7a2b172767dcc9a3203394 in 
/entrypoint.sh \"]}}"
  },
  {
 "v1Compatibility": 
"{\"id\":\"262f434a6db5dfc7e5088923b4eed39b6e837f160a403de168d255782896ca8c\",\"parent\":\"8da0a1a131a17ca30fe02d5a2c5c64ddd7ca7401efc4c81129b1740ba49a216b\",\"created\":\"2017-01-18T05:13:58.66681134Z\",\"container_config\":{\"Cmd\":[\"/bin/sh
 -c #(nop)  EXPOSE 5000/tcp\"]},\"throwaway\":true}"
  },
  {
 "v1Compatibility": 
"{\"id\":\"8da0a1a131a17ca30fe02d5a2c5c64ddd7ca7401efc4c81129b1740ba49a216b\",\"parent\":\"343c0e0e9a064d07213c8a9026626f4ca3b3f12ce9b955b92d5114862675ce3c\",\"created\":\"2017-01-18T05:13:57.905190944Z\",\"container_config\":{\"Cmd\":[\"/bin/sh
 -c #(nop)  VOLUME [/var/lib/registry]\"]},\"throwaway\":true}"
  },
  {
 "v1Compatibility": 
"{\"id\":\"343c0e0e9a064d07213c8a9026626f4ca3b3f12ce9b955b92d5114862675ce3c\",\"parent\":\"f65507bfd5af91b293780b3a955f3b5a1e4f3fa58bcfb6fbfd56814805db1f9c\",\"created\":\"2017-01-18T05:13:53.583988605Z\",\"container_config\":{\"Cmd\":[\"/bin/sh
 -c #(nop) COPY 
file:6c4758d509045dc45381fa2df2e7ffcc661afcaa29805c75f8f1976f2b016db8 in 
/etc/docker/registry/config.yml \"]}}"
  },
  {
 "v1Compatibility": 
"{\"id\":\"f65507bfd5af91b293780b3a955f3b5a1e4f3fa58bcfb6fbfd56814805db1f9c\",\"parent\":\"38535c2527ab54323dd4d430bb9e5e0f95b66faf27f0fd70c794efbf6979fbdc\",\"created\":\"2017-01-18T05:13:52.69446Z\",\"container_config\":{\"Cmd\":[\"/bin/sh
 -c #(nop) COPY 
file:6ccb0558ad0a49a3f19a99428209cf72cb67a21381c8e4286365d5e0aebebd50 in 
/bin/registry \"]}}"
  },
  {
 "v1Compatibility": 
"{\"id\":\"38535c2527ab54323dd4d430bb9e5e0f95b66faf27f0fd70c794efbf6979fbdc\",\"parent\":\"6f8afccfec171d02b03a207536fcc5585825c5161a2923f2e9043e61cab97343\",\"created\":\"2016-12-27T21:38:19.807455414Z\",\"container_config\":{\"Cmd\":[\"/bin/sh
 -c set -ex 

Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.

[Alex Clemmer]

> On Jan. 25, 2017, 12:27 a.m., Joseph Wu wrote:
> > 3rdparty/stout/cmake/StoutConfigure.cmake, line 47
> > 
> >
> > Why not use `*.hpp` instead?
> 
> Alex Clemmer wrote:
> Generally, we also would like to include .h files generated by protobufs, 
> either those that exist, or those that might exist in the future. In Stout, 
> for example, there is `protobuf_tests.proto` in the `tests/` folder. The 
> Stout headers don't have any .proto files that I know of right now, but I 
> think it's better just to use this one pattern everywhere.
> 
> Joseph Wu wrote:
> Ok, but let's be explicit, say `*.h(pp)?`.  Otherwise, we'll be matching 
> against `.hooligans` and `.hoopla` :)
> 
> Similar below.
> 
> Joseph Wu wrote:
> Nevermind.  I didn't realize this was a glob expression, rather than a 
> regex.

Ah, yes. I would have done it that way if I could have made it work.


- Alex


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


On Jan. 17, 2017, 6:41 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55600/
> ---
> 
> (Updated Jan. 17, 2017, 6:41 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout currently manually groups its source code by folder in order to
> present the source nicely in IDEs like XCode and Visual Studio.
> 
> With the recently-introduced CMake function, `GROUP_SOURCE`, this
> process is automated away. This commit will remove these manual groups
> and replace it with a call to this function.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> bc27ac687bae4e1798eece562027ba33c6b32348 
>   3rdparty/stout/tests/CMakeLists.txt 
> a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae 
> 
> Diff: https://reviews.apache.org/r/55600/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56028: Consistently add the FollowSymlink stat argument on Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56027, 56028]

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

- Mesos Reviewbot


On Jan. 27, 2017, 6:12 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56028/
> ---
> 
> (Updated Jan. 27, 2017, 6:12 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer, Joris Van Remoortere, Joseph Wu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7021
> https://issues.apache.org/jira/browse/MESOS-7021
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Consistently add the FollowSymlink stat argument on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> e7f471b828d96bb5cd01823dd78c8570730712ea 
> 
> Diff: https://reviews.apache.org/r/56028/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 56027: Consistently add the FollowSymlink stat argument on POSIX.

[James Peach]

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

Review request for mesos, Alex Clemmer, Joris Van Remoortere, Joseph Wu, and 
Jiang Yan Xu.


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


Repository: mesos


Description
---

Consistently add the FollowSymlink stat argument on POSIX.


Diffs
-

  3rdparty/stout/include/stout/os/posix/stat.hpp 
e7440a4dab82ca3758a60a8fdc859476b2ee5693 
  3rdparty/stout/include/stout/os/stat.hpp 
5c4fd4e630459059fa94c9f98e0d9b73d1280918 

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Review Request 56028: Consistently add the FollowSymlink stat argument on Windows.

[James Peach]

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

Review request for mesos, Alex Clemmer, Joris Van Remoortere, Joseph Wu, and 
Jiang Yan Xu.


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


Repository: mesos


Description
---

Consistently add the FollowSymlink stat argument on Windows.


Diffs
-

  3rdparty/stout/include/stout/os/windows/stat.hpp 
e7f471b828d96bb5cd01823dd78c8570730712ea 

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 56017: WIP: Added a helper for building a task status from an existing one.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56016, 56017]

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

- Mesos Reviewbot


On Jan. 27, 2017, 12:54 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> ---
> 
> (Updated Jan. 27, 2017, 12:54 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28 
>   src/common/protobuf_utils.cpp 3ccda1ac2e94ef3fc20d6a2cdb94b6620d914bcd 
> 
> Diff: https://reviews.apache.org/r/56017/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55876: Avoided shadowing in `Slave::run()`.

[Neil Conway]

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


Fix it, then Ship it!




Variable naming still seems inconsistent, but I see we use both `var_` and 
`_var` in `slave.cpp`. Would be nice to be consistent but we can defer that to 
later.


src/slave/slave.cpp (line 1580)


If we're going to change this, there are a few other places that should 
also be changed: e.g., lines 1179, 1226, 1580, potentially 5292.


- Neil Conway


On Jan. 27, 2017, 11:52 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> ---
> 
> (Updated Jan. 27, 2017, 11:52 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 
> 
> Diff: https://reviews.apache.org/r/55876/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 54215: Fixed duplicate image layer ids returned by docker store.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp (lines 293 
- 296)


This comment here is not quite descripative. I'd suggest use the reply you 
wrote to me in the review board.

ALso, make sure to paste the link to docker code.



src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp (line 301)


I'd add some comment about the order of `fslayers` here. whether it is:
```
[parent, child]
```
or
```
[child, parent]
```

Since child layer can overwrite contents in the parent layer, I think what 
backend expects is `[parent, child]`.

That means fslayers here is in `[child, parent]` order. Now, can you verify 
that the overwriting is ok. I'd improve the comment here.


- Jie Yu


On Jan. 26, 2017, 1:37 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> ---
> 
> (Updated Jan. 26, 2017, 1:37 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian 
> Zhang, and Zhitao Li.
> 
> 
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> cecf34a23329a64fdbce7de4b83827a30975e9a4 
> 
> Diff: https://reviews.apache.org/r/54215/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested by the unit test 
> `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
> 
> Manually tested using the `mesosphere/inky` image, which contains duplicate 
> layer ids.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 56024: Added Dmitry Zhuk to contributors.

[Dmitry Zhuk]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added Dmitry Zhuk to contributors.


Diffs
-

  docs/contributors.yaml 2f8ec11d449dedaee325dda56fadaacebcdeb014 

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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 55767: Ensured task's check status is delivered with reconciliation updates.

[Alexander Rukletsov]

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

(Updated Jan. 27, 2017, 3:38 p.m.)


Review request for mesos, Gastón Kleiman, haosdent huang, and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28 
  src/common/protobuf_utils.cpp 3ccda1ac2e94ef3fc20d6a2cdb94b6620d914bcd 
  src/master/master.cpp 0f2c7cd32161576fad742ef350dff64874b80854 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 55884: Updated the naming of unacknowledged tasks in executors for clarity.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55884]

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

- Mesos Reviewbot


On Jan. 27, 2017, 12:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55884/
> ---
> 
> (Updated Jan. 27, 2017, 12:10 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
> 
> Diff: https://reviews.apache.org/r/55884/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55876: Avoided shadowing in `Slave::run()`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55876]

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

- Mesos Reviewbot


On Jan. 27, 2017, 11:52 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> ---
> 
> (Updated Jan. 27, 2017, 11:52 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 
> 
> Diff: https://reviews.apache.org/r/55876/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 56017: WIP: Added a helper for building a task status from an existing one.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28 
  src/common/protobuf_utils.cpp 3ccda1ac2e94ef3fc20d6a2cdb94b6620d914bcd 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Review Request 56016: Expanded a comment about TaskStatus protobuf.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  include/mesos/mesos.proto 37a97f6bb3c229c0ae16a263e4de458010fba32b 
  include/mesos/v1/mesos.proto 31563b79010fff4c07b1c0ab691c0c7635035845 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 55884: Updated the naming of unacknowledged tasks in executors for clarity.

[Alexander Rukletsov]

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

(Updated Jan. 27, 2017, 12:10 p.m.)


Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 55876: Avoided shadowing in `Slave::run()`.

[Alexander Rukletsov]

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

(Updated Jan. 27, 2017, 11:52 a.m.)


Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Neil Conway.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 55900: Improved style in `HealthChecker`.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Jan. 25, 2017, 6:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55900/
> ---
> 
> (Updated Jan. 25, 2017, 6:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked `_taskPid` in the constructor as const, to be consistent with the
> other parameters.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 6e558f2061a9e31157c47d31cb64b3a8568aace3 
>   src/checks/health_checker.cpp 50aa2858e807b27bbab58a3618f5200cfe4eca9e 
> 
> Diff: https://reviews.apache.org/r/55900/diff/
> 
> 
> Testing
> ---
> 
> `make check` (macOS and Linux)
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55900: Improved style in `HealthChecker`.

[Alexander Rukletsov]

> On Jan. 25, 2017, 6:09 p.m., haosdent huang wrote:
> > I remember we have special reason to use `Option` here instead of `const 
> > Option&` in previous patches. @alex may remember that, or I 
> > missunderstanding something here?
> 
> Gastón Kleiman wrote:
> The parameter was introduced here: https://reviews.apache.org/r/51379/
> 
> I don't see this topic discussed in the review, so I'll let @alexr chime 
> in to see if he remembers something =).

I don't remember anything specific.


- Alexander


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


On Jan. 25, 2017, 6:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55900/
> ---
> 
> (Updated Jan. 25, 2017, 6:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked `_taskPid` in the constructor as const, to be consistent with the
> other parameters.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 6e558f2061a9e31157c47d31cb64b3a8568aace3 
>   src/checks/health_checker.cpp 50aa2858e807b27bbab58a3618f5200cfe4eca9e 
> 
> Diff: https://reviews.apache.org/r/55900/diff/
> 
> 
> Testing
> ---
> 
> `make check` (macOS and Linux)
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55876: Avoided shadowing in `Slave::run()`.

[Alexander Rukletsov]

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

(Updated Jan. 27, 2017, 11:39 a.m.)


Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Neil Conway.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 

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


Testing
---

make check


Thanks,

Alexander Rukletsov