Re: Review Request 56530: Prevent offers for old agents being sent to MULTI_ROLE frameworks.

[Jay Guo]

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

(Updated Feb. 15, 2017, 2:44 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


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


Repository: mesos


Description
---

In a mixed cluster, when an old agent (not MULTI_ROLE capable)
registers with new master (MULTI_ROLE capable), it cannot correctly
handle tasks from a MULTI_ROLE framework. Therefore, we should
disallow offers from these agents being sent to MULTI_ROLE frameworks.


Diffs (updated)
-

  src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
  src/tests/master_tests.cpp c8ef9d6bd9f4f8d631ac87d5e5f8dbe679a70fd1 

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


Testing
---

./bin/mesos-tests.sh 
--gtest_filter="MasterTest.MultiRoleFrameworkDoesNotReceiveOfferFromOldAgent" 
--gtest_break_on_failure --gtest_repeat=-1


Thanks,

Jay Guo

Re: Review Request 56692: Silenced a GMock warning in a test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56692]

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 Feb. 15, 2017, 12:05 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56692/
> ---
> 
> (Updated Feb. 15, 2017, 12:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Silenced a GMock warning in a test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
> 
> Diff: https://reviews.apache.org/r/56692/diff/
> 
> 
> Testing
> ---
> 
> Without this change, the test consistently produces this warning on my system:
> 
> ```
> GMOCK WARNING:
> Uninteresting mock function call - returning directly.
> Function call: shutdown(0x7fe1f0e7d330)
> ```
> 
> With this change, the warning was not observed in 500 test runs.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56689: Windows: Prevented crash if sandbox `stdout` file already exists.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55022, 55023, 55024, 55030, 55162, 55543, 55546, 55547, 
55549, 0, 55749, 56504, 56505, 56591, 56592, 56593, 56594, 56652, 56675, 
56689]

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 Feb. 14, 2017, 11:40 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56689/
> ---
> 
> (Updated Feb. 14, 2017, 11:40 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Subprocess currently provides a cross-platform API that allows the child
> process to redirect `stdin`, `stdout`, and `stderr`, with support for
> file paths, file descriptors, pipes, or Windows file `HANDLE`s.
> 
> Currently, in the case of file paths, the Windows code will use
> `CreateFile` to open the file, with the semantics that it will error out
> if the file already exists (because of the `CREATE_NEW` flag), and
> explicitly specifying that it is ok to have multiple processes writing
> to the file (because of the `FILE_SHARE_WRITE` flag).
> 
> The former of these is undesirable; libprocess should be able to proceed
> regardless of whether the file already exists. But this also causes bugs
> to the downstream, namely Mesos, in which a Fetcher might create the
> sandbox `stdout` folder, and then subsequently crash when it tries to
> open the `stdout` folder in the executor.
> 
> In this commit, we change the `CREATE_NEW` flag to `OPEN_ALWAYS`, which
> has the semantics that we will create the file if it does not exist, and
> open it if it does.
> 
> Additionally, since the documentation does not specify whether
> `FILE_SHARE_WRITE` also allows other processes to have read access, we
> additionally specify the flag `FILE_SHARE_READ` just to be sure we make
> no assumptions about who is able to read and write to these files. See
> comments for additional context on this point.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_windows.cpp 
> 1b5ae5e3ee3c08d05afa1bc533542f42fe26eed0 
> 
> Diff: https://reviews.apache.org/r/56689/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56645: Added agent capabilities to `/state`(v0) endpoint of agent.

[Jay Guo]

> On Feb. 14, 2017, 5:57 p.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, lines 1246-1247
> > 
> >
> > I see we have a type `SlaveInfo::Capability` defined, but don't store 
> > any values there. Shouldn't we have a repeated field there (similar to what 
> > we currently have for e.g., agent attributes, instead of synthetizing 
> > something on the spot here?
> 
> Benjamin Bannier wrote:
> Per our offline discussion, not storing this information in `SlaveInfo` 
> is deliberate and follows the pattern already in place for version which is 
> also communicated via `RegisterSlaveMessage`s instead of configured in 
> `SlaveInfo`. As hardcoding the same thing in two different places doesn't 
> seem too resilent, do you think it would make sense to at least introduce a 
> constant for agent capabilities that we could reference in both `slave/http`, 
> and agent registration code (the version is already taken from a constant 
> `MESOS_VERSION`)?

What do you mean by _two different places_? I think we only store them in 
slave.capabilities per previous commit, no?


- Jay


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


On Feb. 14, 2017, 6:19 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56645/
> ---
> 
> (Updated Feb. 14, 2017, 6:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent capabilities to `/state`(v0) endpoint of agent.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 3d5ab59ddc4dce4d791c1b439f5e1459d1a724a4 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
>   src/master/http.cpp caa1fabb7a709d993ff711cc7803dd05000fa987 
>   src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
>   src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da 
> 
> Diff: https://reviews.apache.org/r/56645/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56587: Fixed a crash on the master upon receiving an invalid inverse offer.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56587]

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 Feb. 14, 2017, 8:22 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56587/
> ---
> 
> (Updated Feb. 14, 2017, 8:22 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-7119
> https://issues.apache.org/jira/browse/MESOS-7119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The erroneous invariant check for `slaveId` can be trigerred when
> the master accepts an invalid inverse offer or when the inverse offer
> has been already rescinded.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
>   src/tests/master_maintenance_tests.cpp 
> 9861d1186e046a4fd547e73825c02b71eedd5ce2 
> 
> Diff: https://reviews.apache.org/r/56587/diff/
> 
> 
> Testing
> ---
> 
> make check + added a test that used to crash before.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51624: Implemented 'GetAgent' call in v1 agent API.

[haosdent huang]

> On Jan. 31, 2017, 11:45 p.m., Vinod Kone wrote:
> > Ship It!
> 
> Vinod Kone wrote:
> Can you commit this @haosdent?

Sure, let me commit.


- haosdent


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


On Oct. 8, 2016, 12:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51624/
> ---
> 
> (Updated Oct. 8, 2016, 12:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6123
> https://issues.apache.org/jira/browse/MESOS-6123
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented 'GetAgent' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 5b91677487f8e23301e67af14c2115c6b4a533ac 
>   include/mesos/v1/agent/agent.proto 8145669073553dc8aa56cfe2c0a0b756d70fee0e 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
>   src/slave/slave.hpp 13c76d1eaea2b49519948c9116e5db5caf9407ea 
>   src/slave/validation.cpp a9f318214182a87783a985cd8495a0ec00c95378 
>   src/tests/api_tests.cpp 26f99f7c337fbbc5278d1b30d3d5c8f659ddf5ca 
> 
> Diff: https://reviews.apache.org/r/51624/diff/
> 
> 
> Testing
> ---
> 
> Added a new test case `AgentAPITest.GetAgent`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51624: Implemented 'GetAgent' call in v1 agent API.

[Vinod Kone]

> On Jan. 31, 2017, 11:45 p.m., Vinod Kone wrote:
> > Ship It!

Can you commit this @haosdent?


- Vinod


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


On Oct. 8, 2016, 12:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51624/
> ---
> 
> (Updated Oct. 8, 2016, 12:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6123
> https://issues.apache.org/jira/browse/MESOS-6123
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented 'GetAgent' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 5b91677487f8e23301e67af14c2115c6b4a533ac 
>   include/mesos/v1/agent/agent.proto 8145669073553dc8aa56cfe2c0a0b756d70fee0e 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
>   src/slave/slave.hpp 13c76d1eaea2b49519948c9116e5db5caf9407ea 
>   src/slave/validation.cpp a9f318214182a87783a985cd8495a0ec00c95378 
>   src/tests/api_tests.cpp 26f99f7c337fbbc5278d1b30d3d5c8f659ddf5ca 
> 
> Diff: https://reviews.apache.org/r/51624/diff/
> 
> 
> Testing
> ---
> 
> Added a new test case `AgentAPITest.GetAgent`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 56591: Stout: Removed MSVC compiler warnings.

[Joseph Wu]

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




3rdparty/stout/include/stout/abort.hpp (lines 45 - 54)


I'd expand the comment with:

`strlen` always returns a positive result, which means it is safe to cast 
it to an unsigned value.



3rdparty/stout/include/stout/duration.hpp (lines 99 - 100)


See: https://reviews.apache.org/r/53708/#comment229180



3rdparty/stout/include/stout/gzip.hpp (line 126)


Note to self: The changes to this file supercede the changes (+ review 
comments) from Daniel's review: https://reviews.apache.org/r/53709/



3rdparty/stout/include/stout/os/windows/dup.hpp (lines 41 - 43)


Do you want to reference/create a JIRA like 
https://issues.apache.org/jira/browse/MESOS-6817 to track unicode?



3rdparty/stout/include/stout/os/windows/environment.hpp (line 28)


I can squash this change into  https://reviews.apache.org/r/55547/ before 
committing.



3rdparty/stout/include/stout/protobuf.hpp (lines 178 - 180)


Hm...

This is a `size_t` (unsigned int) to `int` implicit cast.  This means, if 
the string is of size 2^31 or greater, we will be passing in a negative size to 
the constructor.

In proto2, the maximum size of a protobuf is 64 MB, so we're unlikely to 
hit the upper limit.  

However, since it is possible to pass in an arbitrary string to this 
method, we should add:

```
CHECK_LE(value.size(), std::numeric_limits::max());
```
^ Possibly need a `static_cast` in the first argument above.



3rdparty/stout/include/stout/protobuf.hpp (lines 280 - 282)


Ditto with the CHECK_LE.



3rdparty/stout/include/stout/windows/os.hpp (lines 56 - 57)


Ditto: question about tracking unicode with a JIRA.  Two locations in this 
file.


- Joseph Wu


On Feb. 14, 2017, 1:05 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56591/
> ---
> 
> (Updated Feb. 14, 2017, 1:05 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Removed MSVC compiler warnings.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/abort.hpp 
> b7dba032f579ead83f4e04c942239e721632eeb4 
>   3rdparty/stout/include/stout/duration.hpp 
> 0a30a09678c20937caa6f094c3c63a326e357932 
>   3rdparty/stout/include/stout/gzip.hpp 
> 7040a3275370e7447e843c2471f35e2ba26166e4 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 
> 1fc4d55393aa617d1b67178c945ac0af91c30c99 
>   3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/fd.hpp 
> fddaaa54deaea5e6ef3947142870c7fef76e76aa 
>   3rdparty/stout/include/stout/protobuf.hpp 
> c0f03bc547cddba29309c429b34a0c1e6015c8ea 
>   3rdparty/stout/include/stout/windows/os.hpp 
> b5172fca96c4151f4b1ebb6d343022558f45fc34 
> 
> Diff: https://reviews.apache.org/r/56591/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.

[Joseph Wu]

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




3rdparty/stout/include/stout/os/windows/environment.hpp (line 27)


Oh, this variable is apparently un-used.  I can remove it before committing.


- Joseph Wu


On Feb. 14, 2017, 10:47 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> ---
> 
> (Updated Feb. 14, 2017, 10:47 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-5880
> https://issues.apache.org/jira/browse/MESOS-5880
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows currently exposes two APIs for managing a process's environment
> variables: a CRT API, and a win32 API. This commit will transition Stout
> to use only the win32 API, and retire its use of the CRT APIs.
> 
> There are many reasons for this, for example:
> 
> * Stout currently uses both the CRT and win32 APIs, but they are
>   incompatible, and this causes real bugs. For example, because
>   `os::setenv` uses the win32 API, but `os::environment` uses the CRT
>   API, it is possible to set an environment variable and then later not
>   see it reflected in the environment. In Mesos this causes many bugs,
>   such as when we expect to set `LIBPROCESS_PORT` in a parent, then
>   spawn a health checker, this port doesn't get picked up.
> * The CRT API is very old, and essentially unmaintained. It should not
>   be used unless it is necessary.
> * It is generally easier to mirror the most common POSIX semantics of
>   environment APIs with the win32 API than it is with the CRT API. For
>   example, the Windows CRT implementation of `setenv` will delete an
>   environment variable if you pass an empty string as value, instead of
>   setting the value to be an empty string, like most modern POSIX
>   implementations. On the other hand, the win32 equivalent,
>   `SetEnvironmentVariable`, does implement this behavior.
> 
> The effort to standardize the Windows code paths essentially involves
> two things:
> 
> (1) Removing `os::raw::environment` from Stout's Windows API.
> 
> `os::raw::environment` is not used by the Windows codepaths, and (for
> reasons above) we want to avoid is accidental use of `environ` on
> Windows in the future, as well.
> 
> While it is possible to simply implement `os::raw::environment` using
> the win32 API `GetEnvironmentStrings`, these return fundamentally
> different types, so the allocation logic would become more complex, and
> the semantics of the function would have to change. Either the user
> would have to allocate a `char**` for the environment, or Stout would
> have to manage a `static char**`, or the function would have to allocate
> memory for the user to `free`. All of these are at odds with the POSIX
> semantics, and since this API is only used on POSIX paths, there is no
> real advantage in this line of inquiry.
> 
> (2) Splitting up the implementation of `os::environment`.
> 
> The POSIX `environ` and Windows `GetEnvironmentStrings` are
> fundamentally different types, and require mostly different processing
> logic to transform them to a `hashmap`. There is no real advantage in
> convoluting this processing code to keep the code common between them.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751 
>   3rdparty/stout/include/stout/os/environment.hpp 
> d8c34999829257bee80b0678f2ee096f4798c62b 
>   3rdparty/stout/include/stout/os/posix/environment.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/raw/environment.hpp 
> b3e82ac8071b41748aeb098b7d5fcc210a1d3c43 
>   3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/os_tests.cpp fc359159afcdb60e4406821e123b6358320b6df8 
> 
> Diff: https://reviews.apache.org/r/55547/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56288: Improved the wording of what's logged on command health check timeouts.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 10, 2017, 11:17 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56288/
> ---
> 
> (Updated Feb. 10, 2017, 11:17 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
> ---
> 
> Improved the wording of what's logged on command health check timeouts.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
> 
> Diff: https://reviews.apache.org/r/56288/diff/
> 
> 
> Testing
> ---
> 
> None, not a functional change.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 56681: Use glog to log EXIT() messages.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56680, 56681]

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 Feb. 14, 2017, 9:04 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56681/
> ---
> 
> (Updated Feb. 14, 2017, 9:04 p.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Bugs: MESOS-7115
> https://issues.apache.org/jira/browse/MESOS-7115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use glog to log EXIT() messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/exit.hpp 
> 762864afd8f86f7e1f439ef6ea7e3965a5f147d5 
> 
> Diff: https://reviews.apache.org/r/56681/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 56692: Silenced a GMock warning in a test.

[Neil Conway]

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

Review request for mesos, Benjamin Bannier and Michael Park.


Repository: mesos


Description
---

Silenced a GMock warning in a test.


Diffs
-

  src/tests/master_validation_tests.cpp 
fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 

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


Testing
---

Without this change, the test consistently produces this warning on my system:

```
GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: shutdown(0x7fe1f0e7d330)
```

With this change, the warning was not observed in 500 test runs.


Thanks,

Neil Conway

Review Request 56689: Windows: Prevented crash if sandbox `stdout` file already exists.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Repository: mesos


Description
---

Subprocess currently provides a cross-platform API that allows the child
process to redirect `stdin`, `stdout`, and `stderr`, with support for
file paths, file descriptors, pipes, or Windows file `HANDLE`s.

Currently, in the case of file paths, the Windows code will use
`CreateFile` to open the file, with the semantics that it will error out
if the file already exists (because of the `CREATE_NEW` flag), and
explicitly specifying that it is ok to have multiple processes writing
to the file (because of the `FILE_SHARE_WRITE` flag).

The former of these is undesirable; libprocess should be able to proceed
regardless of whether the file already exists. But this also causes bugs
to the downstream, namely Mesos, in which a Fetcher might create the
sandbox `stdout` folder, and then subsequently crash when it tries to
open the `stdout` folder in the executor.

In this commit, we change the `CREATE_NEW` flag to `OPEN_ALWAYS`, which
has the semantics that we will create the file if it does not exist, and
open it if it does.

Additionally, since the documentation does not specify whether
`FILE_SHARE_WRITE` also allows other processes to have read access, we
additionally specify the flag `FILE_SHARE_READ` just to be sure we make
no assumptions about who is able to read and write to these files. See
comments for additional context on this point.


Diffs
-

  3rdparty/libprocess/src/subprocess_windows.cpp 
1b5ae5e3ee3c08d05afa1bc533542f42fe26eed0 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 56587: Fixed a crash on the master upon receiving an invalid inverse offer.

[Joseph Wu]

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


Fix it, then Ship it!





src/master/master.cpp (line 4731)


We don't use this `slaveId` after assigning it, anymore.  So remove this 
statement.  And the local variable.


- Joseph Wu


On Feb. 14, 2017, 12:22 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56587/
> ---
> 
> (Updated Feb. 14, 2017, 12:22 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-7119
> https://issues.apache.org/jira/browse/MESOS-7119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The erroneous invariant check for `slaveId` can be trigerred when
> the master accepts an invalid inverse offer or when the inverse offer
> has been already rescinded.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
>   src/tests/master_maintenance_tests.cpp 
> 9861d1186e046a4fd547e73825c02b71eedd5ce2 
> 
> Diff: https://reviews.apache.org/r/56587/diff/
> 
> 
> Testing
> ---
> 
> make check + added a test that used to crash before.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56505: Added Windows support for Docker executor.

[Joseph Wu]

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




src/docker/docker.hpp (lines 41 - 47)


This isn't a default, as you can't change the value without breaking it.



src/docker/executor.hpp (lines 51 - 52)


That last note can be ommitted, as this argument is supplied by the agent, 
even if the Mesos agent is not running in Docker (recommended).  Also, if you 
were to invoke the docker executor in a stand-alone way, this note does not 
make sense.



src/slave/constants.hpp (lines 101 - 102)


Copy-pasted comment :)

Suggestion:
Default UNIX socket (Linux) or Named Pipe (Windows) resource that provides 
CLI access to the Docker daemon.



src/slave/containerizer/docker.cpp (lines 1408 - 1420)


I would like to see this block and the same block in `mesos/launch.cpp` be 
separated into another review.

The pipe naming changes and this environment change (technically a no-op 
because Subprocess on Windows is already enforcing it) are not highly 
inter-related.



src/slave/containerizer/mesos/launch.cpp (lines 101 - 103)


Why is this removed from the `#ifdef`?  It still isn't used anywhere on 
Windows.


- Joseph Wu


On Feb. 14, 2017, 11:06 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56505/
> ---
> 
> (Updated Feb. 14, 2017, 11:06 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-3098
> https://issues.apache.org/jira/browse/MESOS-3098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will introduce the changes to the Mesos Docker interfaces
> required to use the Docker executor to run Windows Containers in the
> Mesos agent. In particular, since Windows Containers use named pipes to
> connect do the Docker host, rather than a socket, we introduce the
> plumbing to default to a named pipe connection when invoking the
> `docker` command.
> 
> This work constitutes the beginning of the end of the work that will
> eventually result in Mesos support of Windows Containers.
> 
> This review is partially based on r/52364, which was the work of Daniel
> Pravat.
> 
> Lastly, this review has a planned regression in MESOS-6816. In other
> parts of the codebase, we copy all environment variables from the system
> before launching a process, overwriting user specified environment
> variables. This is not correct behavior, but a half-measure that is
> necessary, because Windows does not inherit environment variables by
> default. In this commit, we make this behavior uniform across all places
> where we are creating a process, because it is better for behavior to be
> consistent before we get around to fixing it. We do have concrete plans
> to come back and resolve this issue.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c 
>   src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
>   src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/slave/containerizer/mesos/launch.hpp 
> 49eb9128ae5a1e4ec02e2b9d9e3cb67d7a8f7663 
>   src/slave/containerizer/mesos/launch.cpp 
> 4dd81b47ca4654f5e783a4f2227834e938bc8bb3 
>   src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 
>   src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 
> 
> Diff: https://reviews.apache.org/r/56505/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56504: Stout: Made `os::mktemp` fully cross-platform.

[Alex Clemmer]

> On Feb. 14, 2017, 11:05 p.m., Joseph Wu wrote:
> > When we switched `mkdtemp` (directory version), that exposed some odd 
> > behavior on OSX due to how lengthy OSX's default temporary directory is.  
> > The IO Switchboard tests there ran into a 108 character limit for Unix 
> > sockets.  It appears that this patch does not have as many side effects (as 
> > we create fewer temporary files than we do temporary directories).

I should have written that I tested this in OS X also. Thanks for being 
thorough!


- Alex


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


On Feb. 9, 2017, 5:57 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56504/
> ---
> 
> (Updated Feb. 9, 2017, 5:57 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Made `os::mktemp` fully cross-platform.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mktemp.hpp 
> 231234f7652937e042f49b13fe8fc3c7193d26e1 
> 
> Diff: https://reviews.apache.org/r/56504/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56367: Stout: Windows: patch `os::killtree` to terminate job objects.

[Andrew Schwartzmeyer]

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



Test again review bot!

- Andrew Schwartzmeyer


On Feb. 8, 2017, 9:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56367/
> ---
> 
> (Updated Feb. 8, 2017, 9:14 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before we deprecate `os::killtree`, we need to it at least work
> with the new job object semantics of the `WindowsLauncher`.
> Given the PID of the "process tree" to kill, this maps it to the
> name used for the job object, opens a new handle to it, and calls
> `os::kill_job` to terminate it.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> 2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 
> 
> Diff: https://reviews.apache.org/r/56367/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56368: Libprocess: Windows: Remove `CREATE_JOB` parent hook.

[Andrew Schwartzmeyer]

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



Test again review bot!

- Andrew Schwartzmeyer


On Feb. 8, 2017, 9:15 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56368/
> ---
> 
> (Updated Feb. 8, 2017, 9:15 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Windows: Remove `CREATE_JOB` parent hook.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> 7117a2730ae8666d0b55cbe05162a0b4e843193c 
>   3rdparty/libprocess/src/subprocess.cpp 
> bc1b99b8f84332f42623c35fc2b8d5186ba8af70 
> 
> Diff: https://reviews.apache.org/r/56368/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56504: Stout: Made `os::mktemp` fully cross-platform.

[Joseph Wu]

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


Ship it!




When we switched `mkdtemp` (directory version), that exposed some odd behavior 
on OSX due to how lengthy OSX's default temporary directory is.  The IO 
Switchboard tests there ran into a 108 character limit for Unix sockets.  It 
appears that this patch does not have as many side effects (as we create fewer 
temporary files than we do temporary directories).

- Joseph Wu


On Feb. 9, 2017, 9:57 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56504/
> ---
> 
> (Updated Feb. 9, 2017, 9:57 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Made `os::mktemp` fully cross-platform.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mktemp.hpp 
> 231234f7652937e042f49b13fe8fc3c7193d26e1 
> 
> Diff: https://reviews.apache.org/r/56504/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56052: Added the 'Secret' protobuf message.

[Albert Strasheim]

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




include/mesos/mesos.proto (line 1994)


I believe pretty strongly that this should be bytes. Discussed with Greg.

If I put bytes into a secret, I want to get bytes out.

If my secret has a bunch of null bytes in the middle of it, it might not be 
suitable for passing to the task as an env var, and I should use a file instead.

I shouldn't have to change my app or write a script to transform secrets 
values.


- Albert Strasheim


On Feb. 9, 2017, 6:33 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56052/
> ---
> 
> (Updated Feb. 9, 2017, 6:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-6996
> https://issues.apache.org/jira/browse/MESOS-6996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `Secret` protobuf message which
> is designed to serve as a generic mechanism for passing
> priviledged information within Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
> 
> Diff: https://reviews.apache.org/r/56052/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56675: Silence MSVC compiler warnings in libmesos.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55022, 55023, 55024, 55030, 55162, 55543, 55546, 55547, 
55549, 0, 55749, 56504, 56505, 56591, 56592, 56593, 56594, 56652, 56675]

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 Feb. 14, 2017, 6:40 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56675/
> ---
> 
> (Updated Feb. 14, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Silence MSVC compiler warnings in libmesos.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee 
>   src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ae49fcd659972f984238f8f90aa395e345bfaaa6 
>   src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd 
>   src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 627004663cbd7223a252b875c51454a20e645be6 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5114c130efbfb252dde1e85c081f5174e66f57af 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
>   src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382 
>   src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5 
>   src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc 
>   src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f 
>   src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59 
>   src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da 
>   src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
>   src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8 
> 
> Diff: https://reviews.apache.org/r/56675/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55749: Added CMake to standard documentation.

[Joseph Wu]

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


Ship it!




Whitespace nits that I can fix before committing.


docs/configuration-cmake.md (line 2)


CMake Configuration



docs/configuration-cmake.md (line 9)


We're extremely inconsistent about this in the documentation, but we try to 
stick to a 80 character soft limit where possible.


- Joseph Wu


On Feb. 13, 2017, 6:18 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55749/
> ---
> 
> (Updated Feb. 13, 2017, 6:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-3139
> https://issues.apache.org/jira/browse/MESOS-3139
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CMake to standard documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration-cmake.md PRE-CREATION 
>   docs/configuration.md 656aaa34915eaee91d388febbc7574287b9f51b5 
>   docs/home.md 091dae20b0a26aac03186edaefab93c54b3063b4 
> 
> Diff: https://reviews.apache.org/r/55749/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.

[Joseph Wu]

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


Ship it!




A few minor nits below that I will fix before committing.


3rdparty/stout/include/stout/os/raw/environment.hpp (line 36)


I think the capitalization of `win32` should be `Win32`.  That's how it 
appears in the MSDN docs :)



3rdparty/stout/include/stout/os/raw/environment.hpp (line 82)


Even if the condition is `ifndef`, we don't add an exclamation in the 
comment.  The only exception is if we have a block like:
```
#if !defined(__WINDOWS__) || defined(__linux__)
```

In that case, the ending comment would be:
```
#endif // !__WINDOWS || __linux__
```



3rdparty/stout/include/stout/os/raw/environment.hpp (line 104)


Similar here.



3rdparty/stout/include/stout/os/windows/environment.hpp (line 24)


For code-reading sanity, I'm going to add this comment block:
```
  // NOTE: The `GetEnvironmentStrings` call returns a pointer to a 
  // read-only block of memory with the following format (minus newlines):
  // Var1=Value1\0
  // Var2=Value2\0
  // Var3=Value3\0
  // ...
  // VarN=ValueN\0\0
```



3rdparty/stout/include/stout/os/windows/environment.hpp (line 28)


Missing space after the `for`



3rdparty/stout/include/stout/os/windows/environment.hpp (line 30)


For code-reading sanity:

// Increment past the current environment string and null terminator.


- Joseph Wu


On Feb. 14, 2017, 10:47 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> ---
> 
> (Updated Feb. 14, 2017, 10:47 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-5880
> https://issues.apache.org/jira/browse/MESOS-5880
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows currently exposes two APIs for managing a process's environment
> variables: a CRT API, and a win32 API. This commit will transition Stout
> to use only the win32 API, and retire its use of the CRT APIs.
> 
> There are many reasons for this, for example:
> 
> * Stout currently uses both the CRT and win32 APIs, but they are
>   incompatible, and this causes real bugs. For example, because
>   `os::setenv` uses the win32 API, but `os::environment` uses the CRT
>   API, it is possible to set an environment variable and then later not
>   see it reflected in the environment. In Mesos this causes many bugs,
>   such as when we expect to set `LIBPROCESS_PORT` in a parent, then
>   spawn a health checker, this port doesn't get picked up.
> * The CRT API is very old, and essentially unmaintained. It should not
>   be used unless it is necessary.
> * It is generally easier to mirror the most common POSIX semantics of
>   environment APIs with the win32 API than it is with the CRT API. For
>   example, the Windows CRT implementation of `setenv` will delete an
>   environment variable if you pass an empty string as value, instead of
>   setting the value to be an empty string, like most modern POSIX
>   implementations. On the other hand, the win32 equivalent,
>   `SetEnvironmentVariable`, does implement this behavior.
> 
> The effort to standardize the Windows code paths essentially involves
> two things:
> 
> (1) Removing `os::raw::environment` from Stout's Windows API.
> 
> `os::raw::environment` is not used by the Windows codepaths, and (for
> reasons above) we want to avoid is accidental use of `environ` on
> Windows in the future, as well.
> 
> While it is possible to simply implement `os::raw::environment` using
> the win32 API `GetEnvironmentStrings`, these return fundamentally
> different types, so the allocation logic would become more complex, and
> the semantics of the function would have to change. Either the user
> would have to allocate a `char**` for the environment, or Stout would
> have to manage a `static char**`, or the function would have to allocate
> memory for the user to `free`. All of these are at odds with the POSIX
> semantics, and since this API is only used on POSIX paths, there is no
> real advantage in this line of inquiry.
> 
> (2) Splitting up the implementation of `os::environment`.
> 
> The POSIX `environ` and Windows `GetEnvironmentStrings` are
> fundamentally different types, and require mostly different processing
> logic to

Review Request 56680: Apply glog to agent exit messages.

[James Peach]

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

Review request for mesos and haosdent huang.


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


Repository: mesos


Description
---

EXIT(...) doesn't format messages like glog so it's less easy to
tell where a message came from and when it was emitted. For agent all
initialization messages, perform a LOG(ERROR) followe by an exit. For
other agent exit scenarios, use LOG(FATAL) or the equivalent CHECK().


Diffs
-

  src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Review Request 56681: Use glog to log EXIT() messages.

[James Peach]

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

Review request for mesos and haosdent huang.


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


Repository: mesos


Description
---

Use glog to log EXIT() messages.


Diffs
-

  3rdparty/stout/include/stout/exit.hpp 
762864afd8f86f7e1f439ef6ea7e3965a5f147d5 

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56493]

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 Feb. 14, 2017, 6:35 p.m., Pierre Cheynier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> ---
> 
> (Updated Feb. 14, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
> https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net//speed`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> ---
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56213: Added check tests for command executor.

[Gastón Kleiman]

> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp, line 177
> > 
> >
> > s/agent/slave/ 
> > 
> > here and everywhere else.
> > 
> > we are not doing this change yet.

We do this in `health_check_tests.cpp`:

```
$ grep "agent = " health_check_tests.cpp | wc -l
  15
$ grep "slave = " health_check_tests.cpp | wc -l
   0
$
```


- Gastón


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


On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> ---
> 
> (Updated Feb. 8, 2017, 4:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe 
> 
> Diff: https://reviews.apache.org/r/56213/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56587: Fixed a crash on the master upon receiving an invalid inverse offer.

[Anand Mazumdar]

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

(Updated Feb. 14, 2017, 8:22 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Review comments.


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


Repository: mesos


Description
---

The erroneous invariant check for `slaveId` can be trigerred when
the master accepts an invalid inverse offer or when the inverse offer
has been already rescinded.


Diffs (updated)
-

  src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
  src/tests/master_maintenance_tests.cpp 
9861d1186e046a4fd547e73825c02b71eedd5ce2 

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


Testing
---

make check + added a test that used to crash before.


Thanks,

Anand Mazumdar

Re: Review Request 56587: Fixed a crash on the master upon receiving an invalid inverse offer.

[Joseph Wu]

> On Feb. 13, 2017, 11:51 a.m., Joseph Wu wrote:
> > src/master/master.cpp, lines 4756-4777
> > 
> >
> > Moving the error above the CHECK leaves out the log message in one case:
> > 
> > * ACCEPT_INVERSE_OFFER is called with multiple offers (invalid-offer-id 
> > and valid-offer-id).
> > * In the processing loop, we will LOG(WARNING) for `invalid-offer-id`.
> > * After the processing loop, we _should_ LOG(INFO) for `valid-offer-id` 
> > with the agent and framework info too.  <-- This one is left out in your 
> > patch
> 
> Anand Mazumdar wrote:
> hmm, the semantics that we have currently for the 
> `accept()`/`acceptInverseOffers()` is that even if we have a _single_ invalid 
> offer we treat the entire request as being invalid. This seems like the right 
> behavior to me i.e., we log that the call used invalid offers if any of the 
> offers are invalid. If we feel otherwise though, that seems like a separate 
> orthogonal issue that we should tackle for both the handlers.

It feels right to reject the entire array of offers if any of them are invalid.

The problem with inverse offers is that there is no feedback to the framework.  
If a framework accidentally formats all InverseOfferIDs incorrectly, we have no 
way of indicating this to the framework.  For normal offers, the master can 
send back a status update with TASK_DROPPED/TASK_LOST.
^ This is why we try to process all the valid InverseOfferIDs even if part of 
the request is invalid.


- Joseph


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


On Feb. 12, 2017, 9:44 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56587/
> ---
> 
> (Updated Feb. 12, 2017, 9:44 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-7119
> https://issues.apache.org/jira/browse/MESOS-7119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The erroneous invariant check for `slaveId` can be trigerred when
> the master accepts an invalid inverse offer or when the inverse offer
> has been already rescinded.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
>   src/tests/master_maintenance_tests.cpp 
> 9861d1186e046a4fd547e73825c02b71eedd5ce2 
> 
> Diff: https://reviews.apache.org/r/56587/diff/
> 
> 
> Testing
> ---
> 
> make check + added a test that used to crash before.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56675: Silence MSVC compiler warnings in libmesos.

[Benjamin Bannier]

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



I am not sure what other solutions you explored, but I believe a preferable 
solution would be to produce reasonable types from the start instead of 
`static_cast`'ing at some use sites (that looks like an indication of somewhat 
uncoordinated interfaces).

Also, this looks like a rather ad-hoc workaround specific to MSVC's default 
warnings. I believe a more maintainable solution would be to instead either 
disable that warning in MSVC (if at all possible), or enable that warning or a 
similar one for other compilers as well. That would somewhat limit the 
possibilities of developers primarily on e.g., Linux to introduce similar 
issues for the MSVC again in the future.


src/tests/resources_tests.cpp (line 68)


This does change semantics since neither 45.55f nor 45.55 are exactly 
representable in IEEE-754 form, and `EXPECT_*_EQ` considers numbers within 
4ULPs as equal. We should pick some representable number here instead.

Could you follow up with a cleanup?

Here and below.


- Benjamin Bannier


On Feb. 14, 2017, 7:40 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56675/
> ---
> 
> (Updated Feb. 14, 2017, 7:40 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Silence MSVC compiler warnings in libmesos.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee 
>   src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ae49fcd659972f984238f8f90aa395e345bfaaa6 
>   src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd 
>   src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 627004663cbd7223a252b875c51454a20e645be6 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5114c130efbfb252dde1e85c081f5174e66f57af 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
>   src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382 
>   src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5 
>   src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc 
>   src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f 
>   src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59 
>   src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da 
>   src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
>   src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8 
> 
> Diff: https://reviews.apache.org/r/56675/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56505: Added Windows support for Docker executor.

[Alex Clemmer]

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

(Updated Feb. 14, 2017, 7:06 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Changes
---

Add forgotten change to `launch.hpp`


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


Repository: mesos


Description
---

This commit will introduce the changes to the Mesos Docker interfaces
required to use the Docker executor to run Windows Containers in the
Mesos agent. In particular, since Windows Containers use named pipes to
connect do the Docker host, rather than a socket, we introduce the
plumbing to default to a named pipe connection when invoking the
`docker` command.

This work constitutes the beginning of the end of the work that will
eventually result in Mesos support of Windows Containers.

This review is partially based on r/52364, which was the work of Daniel
Pravat.

Lastly, this review has a planned regression in MESOS-6816. In other
parts of the codebase, we copy all environment variables from the system
before launching a process, overwriting user specified environment
variables. This is not correct behavior, but a half-measure that is
necessary, because Windows does not inherit environment variables by
default. In this commit, we make this behavior uniform across all places
where we are creating a process, because it is better for behavior to be
consistent before we get around to fixing it. We do have concrete plans
to come back and resolve this issue.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c 
  src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
  src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/slave/containerizer/mesos/launch.hpp 
49eb9128ae5a1e4ec02e2b9d9e3cb67d7a8f7663 
  src/slave/containerizer/mesos/launch.cpp 
4dd81b47ca4654f5e783a4f2227834e938bc8bb3 
  src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 
  src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 56657: Add support for local resolution of Docker images.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56657]

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 Feb. 14, 2017, 5:51 p.m., Santhosh Kumar Shanmugham wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56657/
> ---
> 
> (Updated Feb. 14, 2017, 5:51 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7089
> https://issues.apache.org/jira/browse/MESOS-7089
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosContainerizer's local Docker does not have support for
> mutable tags which can point to different immutable digests. This
> indirection is required when an operator wants to rollout updates
> without affecting the customer's configuration.
> 
> Introduce a new optional agent flag `docker_local_resolution_mapping`
> which will point to a JSON-formatted file, that contains mappings,
> to be used for local resolution of docker images.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 656aaa34915eaee91d388febbc7574287b9f51b5 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 68ce265a5d0a61f8d9ed55dd14c630dcf893a7d2 
>   src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
>   src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> ce57c06d50b47a150ff40412c1fde99f16892434 
> 
> Diff: https://reviews.apache.org/r/56657/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Santhosh Kumar Shanmugham
> 
>

Re: Review Request 56212: Added support for general checks to command executor.

[Gastón Kleiman]

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




src/launcher/executor.cpp (lines 848 - 854)


This method is called to build updates when the task transitions to 
`TASK_KILLING`, `TASK_FINISHED`, `TASK_KILLED`, and `TASK_FAILED`.

Do we want to lose the result of the previous check in all those cases? I 
think it might make sense to keep it at least for `TASK_KILLING`. It would also 
make sense to keep the `health` flag in that case.


- Gastón Kleiman


On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> ---
> 
> (Updated Feb. 8, 2017, 4:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 
> 
> Diff: https://reviews.apache.org/r/56212/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.

[Alex Clemmer]

> On Jan. 24, 2017, 2:23 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/raw/environment.hpp, lines 108-120
> > 
> >
> > Should we also move away from functions like `os::execvpe`?  If so, we 
> > would be able to completely exclude `stout/raw/environment.hpp` from the 
> > Windows headers.
> > 
> > `Envp` is currently only used in one location:
> > 
> > https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/src/slave/containerizer/mesos/launch.cpp#L659-L684

Yes, we should. This will probably happen when we start to transition away from 
the hand-rolled windows task launching in the Command Executor.

It probably is out of scope for this review, though, much as I'd like to do it 
now.


> On Jan. 24, 2017, 2:23 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/environment.hpp, lines 22-40
> > 
> >
> > We should refactor this code along with some very similar parsing logic 
> > here:
> > 
> > https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/3rdparty/libprocess/include/process/windows/subprocess.hpp#L90-L114

Per our discussion, we have agreed to clean this up later as part of a 
refactoring of subprocess.


- Alex


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


On Feb. 14, 2017, 6:47 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> ---
> 
> (Updated Feb. 14, 2017, 6:47 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-5880
> https://issues.apache.org/jira/browse/MESOS-5880
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows currently exposes two APIs for managing a process's environment
> variables: a CRT API, and a win32 API. This commit will transition Stout
> to use only the win32 API, and retire its use of the CRT APIs.
> 
> There are many reasons for this, for example:
> 
> * Stout currently uses both the CRT and win32 APIs, but they are
>   incompatible, and this causes real bugs. For example, because
>   `os::setenv` uses the win32 API, but `os::environment` uses the CRT
>   API, it is possible to set an environment variable and then later not
>   see it reflected in the environment. In Mesos this causes many bugs,
>   such as when we expect to set `LIBPROCESS_PORT` in a parent, then
>   spawn a health checker, this port doesn't get picked up.
> * The CRT API is very old, and essentially unmaintained. It should not
>   be used unless it is necessary.
> * It is generally easier to mirror the most common POSIX semantics of
>   environment APIs with the win32 API than it is with the CRT API. For
>   example, the Windows CRT implementation of `setenv` will delete an
>   environment variable if you pass an empty string as value, instead of
>   setting the value to be an empty string, like most modern POSIX
>   implementations. On the other hand, the win32 equivalent,
>   `SetEnvironmentVariable`, does implement this behavior.
> 
> The effort to standardize the Windows code paths essentially involves
> two things:
> 
> (1) Removing `os::raw::environment` from Stout's Windows API.
> 
> `os::raw::environment` is not used by the Windows codepaths, and (for
> reasons above) we want to avoid is accidental use of `environ` on
> Windows in the future, as well.
> 
> While it is possible to simply implement `os::raw::environment` using
> the win32 API `GetEnvironmentStrings`, these return fundamentally
> different types, so the allocation logic would become more complex, and
> the semantics of the function would have to change. Either the user
> would have to allocate a `char**` for the environment, or Stout would
> have to manage a `static char**`, or the function would have to allocate
> memory for the user to `free`. All of these are at odds with the POSIX
> semantics, and since this API is only used on POSIX paths, there is no
> real advantage in this line of inquiry.
> 
> (2) Splitting up the implementation of `os::environment`.
> 
> The POSIX `environ` and Windows `GetEnvironmentStrings` are
> fundamentally different types, and require mostly different processing
> logic to transform them to a `hashmap`. There is no real advantage in
> convoluting this processing code to keep the code common between them.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751 
>   3rdparty/stout/include/stout/os/environment.hpp 
> d8c34999829257bee80b0678f2ee096f47

Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.

[Alex Clemmer]

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

(Updated Feb. 14, 2017, 6:47 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
---

Address Joseph's comments.


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


Repository: mesos


Description
---

Windows currently exposes two APIs for managing a process's environment
variables: a CRT API, and a win32 API. This commit will transition Stout
to use only the win32 API, and retire its use of the CRT APIs.

There are many reasons for this, for example:

* Stout currently uses both the CRT and win32 APIs, but they are
  incompatible, and this causes real bugs. For example, because
  `os::setenv` uses the win32 API, but `os::environment` uses the CRT
  API, it is possible to set an environment variable and then later not
  see it reflected in the environment. In Mesos this causes many bugs,
  such as when we expect to set `LIBPROCESS_PORT` in a parent, then
  spawn a health checker, this port doesn't get picked up.
* The CRT API is very old, and essentially unmaintained. It should not
  be used unless it is necessary.
* It is generally easier to mirror the most common POSIX semantics of
  environment APIs with the win32 API than it is with the CRT API. For
  example, the Windows CRT implementation of `setenv` will delete an
  environment variable if you pass an empty string as value, instead of
  setting the value to be an empty string, like most modern POSIX
  implementations. On the other hand, the win32 equivalent,
  `SetEnvironmentVariable`, does implement this behavior.

The effort to standardize the Windows code paths essentially involves
two things:

(1) Removing `os::raw::environment` from Stout's Windows API.

`os::raw::environment` is not used by the Windows codepaths, and (for
reasons above) we want to avoid is accidental use of `environ` on
Windows in the future, as well.

While it is possible to simply implement `os::raw::environment` using
the win32 API `GetEnvironmentStrings`, these return fundamentally
different types, so the allocation logic would become more complex, and
the semantics of the function would have to change. Either the user
would have to allocate a `char**` for the environment, or Stout would
have to manage a `static char**`, or the function would have to allocate
memory for the user to `free`. All of these are at odds with the POSIX
semantics, and since this API is only used on POSIX paths, there is no
real advantage in this line of inquiry.

(2) Splitting up the implementation of `os::environment`.

The POSIX `environ` and Windows `GetEnvironmentStrings` are
fundamentally different types, and require mostly different processing
logic to transform them to a `hashmap`. There is no real advantage in
convoluting this processing code to keep the code common between them.


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751 
  3rdparty/stout/include/stout/os/environment.hpp 
d8c34999829257bee80b0678f2ee096f4798c62b 
  3rdparty/stout/include/stout/os/posix/environment.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/raw/environment.hpp 
b3e82ac8071b41748aeb098b7d5fcc210a1d3c43 
  3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 
  3rdparty/stout/tests/os_tests.cpp fc359159afcdb60e4406821e123b6358320b6df8 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 56675: Silence MSVC compiler warnings in libmesos.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Repository: mesos


Description
---

Silence MSVC compiler warnings in libmesos.


Diffs
-

  src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee 
  src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
  src/master/allocator/sorter/drf/sorter.cpp 
ae49fcd659972f984238f8f90aa395e345bfaaa6 
  src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd 
  src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72 
  src/slave/containerizer/mesos/containerizer.cpp 
d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
  src/slave/containerizer/mesos/isolators/posix.hpp 
627004663cbd7223a252b875c51454a20e645be6 
  src/slave/containerizer/mesos/launcher.cpp 
5114c130efbfb252dde1e85c081f5174e66f57af 
  src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
  src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382 
  src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5 
  src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc 
  src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f 
  src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59 
  src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da 
  src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
  src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 56210: Reused previous task status to generate a new one in command executor.

[Gastón Kleiman]

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




src/launcher/executor.cpp (line 828)


s/Rewrite/Overwrite/ or Update?


- Gastón Kleiman


On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56210/
> ---
> 
> (Updated Feb. 9, 2017, 12:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a new task status update is generated in the executor, we have
> to make sure specific data is duplicated from the previous update
> to, e.g., avoid shadowing of those data during reconciliation. For
> instance, consider a check status being sent; in this status update
> we must include the latest known health information.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 
> 
> Diff: https://reviews.apache.org/r/56210/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

[Pierre Cheynier]

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

(Updated Feb. 14, 2017, 6:35 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Fixed issues reported by @jieyu.


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


Repository: mesos


Description
---

Since a lot of environments do not provide interface speed in
/sys/class/net//speed`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
f6f2bfe1d5d3c113036ad44a480f97bbb462a269 

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


Testing
---

./configure
make -j 8
make check


Thanks,

Pierre Cheynier

Review Request 56657: Add support for local resolution of Docker images.

[Santhosh Kumar Shanmugham]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

MesosContainerizer's local Docker does not have support for
mutable tags which can point to different immutable digests. This
indirection is required when an operator wants to rollout updates
without affecting the customer's configuration.

Introduce a new optional agent flag `docker_local_resolution_mapping`
which will point to a JSON-formatted file, that contains mappings,
to be used for local resolution of docker images.


Diffs
-

  docs/configuration.md 656aaa34915eaee91d388febbc7574287b9f51b5 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
68ce265a5d0a61f8d9ed55dd14c630dcf893a7d2 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 
  src/tests/containerizer/provisioner_docker_tests.cpp 
ce57c06d50b47a150ff40412c1fde99f16892434 

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


Testing
---

make && make check


Thanks,

Santhosh Kumar Shanmugham

Re: Review Request 56208: Updated checks library with general check support.

[Gastón Kleiman]

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



Many of the comments here also apply to the current code in 
`health_checker.cpp`. One possibility would be to commit this patch without 
fixing them, and then creating a new patch with the fixes for both 
`health_checker.cpp` and `checker.cpp`.


src/checks/checker.hpp (line 61)


Do we need to pass the `TaskID` to the callback? The `checker` is bound to 
a task, so the creator can do something like:

```
checker = Checker::create(
checkInfo,
defer(self(), &Self::checkStatusUpdated, taskId, lambda::_1),
...
);
```



src/checks/checker.hpp (line 77)


`Checker(const Checker&) = delete` seems to be more popular than making the 
constructor private.

Is that idiom preferred?



src/checks/checker.cpp (line 31)


Do we really need `startTime` (see two issues below)? If not, we can remove 
this.



src/checks/checker.cpp (line 65)


Ditto.



src/checks/checker.cpp (line 173)


Looks like a left-over from health checks. There it is used for the grace 
period, but here it is initialized, and then never used again.



src/checks/checker.cpp (line 219)


I think that it'd be useful to log the `taskId` here, since an executor 
might have multiple checkers running at the same time.



src/checks/checker.cpp (line 267)


Ditto `taskId`.



src/checks/checker.cpp (line 305)


Ditto `taskId`.



src/checks/checker.cpp (line 315)


Ditto `taskId`.



src/checks/checker.cpp (lines 339 - 344)


I know that what you put here is what we currently do for command health 
checks, but it would probably make sense to  overwrite env variables set in 
`check.command().command().environment()`.

What we have here seems to be the equivalent of passing `environment = 
None()` to `subprocess()`.



src/checks/checker.cpp (line 351)


Ditto `taskId`.



src/checks/checker.cpp (line 365)


Ditto `taskId`.



src/checks/checker.cpp (line 383)


mark this `const`? or is it considered a POD?



src/checks/checker.cpp (line 394)


Ditto `taskId`.



src/checks/checker.cpp (line 420)


Ditto `taskId`.



src/checks/checker.cpp (line 428)


Ditto `taskId`.



src/checks/checker.cpp (line 450)


Ditto `taskId`.



src/checks/checker.cpp (line 481)


ditto marking it as `const`



src/checks/checker.cpp (line 497)


Ditto `taskId`.



src/checks/checker.cpp (line 570)


Ditto `taskId`.


- Gastón Kleiman


On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> ---
> 
> (Updated Feb. 9, 2017, 12:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
> 
> Diff: https://reviews.apache.org/r/56208/diff/
> 
> 
> Testing
> ---
> 
> Se

Re: Review Request 56611: Relax perf version check for Arch Linux.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56611]

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 Feb. 14, 2017, 3:59 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> ---
> 
> (Updated Feb. 14, 2017, 3:59 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Relax perf version check for Arch Linux.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 9c4330b6086abb18f03660fe89a6fb01d8ed 
>   src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb 
>   src/tests/containerizer/perf_tests.cpp 
> d536ecc5cb24787bc3487efb146573cd4f3ded43 
> 
> Diff: https://reviews.apache.org/r/56611/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56611: Relax perf version check for Arch Linux.

[James Peach]

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

(Updated Feb. 14, 2017, 3:59 p.m.)


Review request for mesos and Neil Conway.


Changes
---

Update tests.


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


Repository: mesos


Description
---

Relax perf version check for Arch Linux.


Diffs (updated)
-

  src/linux/perf.hpp 9c4330b6086abb18f03660fe89a6fb01d8ed 
  src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb 
  src/tests/containerizer/perf_tests.cpp 
d536ecc5cb24787bc3487efb146573cd4f3ded43 

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 56618: Updated common Mesos code to use 'AuthenticationContext'.

[Alexander Rojas]

> On Feb. 14, 2017, 4:12 p.m., Jan Schlicht wrote:
> > src/common/http.hpp, line 132
> > 
> >
> > Why `const` when you're returning a value?

so you cannot assign to the returned value, i.e. you cannot do 
`createAuthorizationObject() = somethingelse`. It is a common C++ idiom.


- Alexander


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


On Feb. 14, 2017, 12:46 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56618/
> ---
> 
> (Updated Feb. 14, 2017, 12:46 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates common Mesos HTTP-related helpers,
> as well as the `authorization::Subject` protobuf
> message, to make use of the `AuthenticationContext`
> type instead of an `Option principal`.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 8b860a3e8e0b1c660a8fefc97f10f5acc0501920 
>   src/common/http.hpp 3d5ab59ddc4dce4d791c1b439f5e1459d1a724a4 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
> 
> Diff: https://reviews.apache.org/r/56618/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

[Jan Schlicht]

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



This look great already. But I'll need more time to deeply review this, e.g. 
I'll do another round, this are the few things I found while quickly looking 
over the patch.


src/files/files.cpp (line 800)


Remove the space between `context` and `)`.



src/master/master.cpp (line 1613)


Check for `context.isSome()` first.



src/master/weights_handler.cpp (line 369)


Remove `<< context.get()` here.


- Jan Schlicht


On Feb. 14, 2017, 1:14 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> ---
> 
> (Updated Feb. 14, 2017, 1:14 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in Mesos
> to accept an `AuthenticationContext` instead of an
> `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
>   src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
>   src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56618: Updated common Mesos code to use 'AuthenticationContext'.

[Jan Schlicht]

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



This look great already. But I'll need more time to deeply review this, e.g. 
I'll do another round, this are the few things I found while quickly looking 
over the patch.


src/common/http.hpp (line 132)


Why `const` when you're returning a value?



src/common/http.hpp (line 133)


How about using an `Option` here and returning 
`Subject()` in the case of `context.isNone()`?
All calls to this functions in the following patch are either
```
authorization::Subject subject = context.isSome()
  ? createAuthorizationSubject(context.get())
  : authorization::Subject();
```
or
```
if (context.isSome()) {
  
request.mutable_subject()->CopyFrom(createAuthorizationSubject(context.get()));
}
```
At least the first form would look much simpler and concise when changing 
the function signature this way:
```
authorization::Subject subject = createAuthorizationSubject(context)
```
What do you think?


- Jan Schlicht


On Feb. 14, 2017, 12:46 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56618/
> ---
> 
> (Updated Feb. 14, 2017, 12:46 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates common Mesos HTTP-related helpers,
> as well as the `authorization::Subject` protobuf
> message, to make use of the `AuthenticationContext`
> type instead of an `Option principal`.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 8b860a3e8e0b1c660a8fefc97f10f5acc0501920 
>   src/common/http.hpp 3d5ab59ddc4dce4d791c1b439f5e1459d1a724a4 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
> 
> Diff: https://reviews.apache.org/r/56618/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56601: Used single quotes instead of backticks in error message.

[Benjamin Bannier]

> On Feb. 14, 2017, 2:48 p.m., Gastón Kleiman wrote:
> > I think that we should also update `checks/checker.cpp` and 
> > `checks/health_checker.cpp`:
> > 
> > ```
> > $ ag --cpp --nomultiline '^\s*[^/"]+".*`'
> > checks/checker.cpp
> > 60:"Check's `CommandInfo` is invalid: " + error->message);
> > 
> > checks/health_checker.cpp
> > :"Health check's `CommandInfo` is invalid: " + 
> > error->message);
> > ```

I didn't intend to make a full sweep in this patch, but rather to address a 
single such instance in the reservation validation code I touched.

I believe a full sweep should fix a much larger number of places (in addition 
to error messages probably also in e.g., help strings),

$ git grep -n '".*`' | grep -v '^.*\/\/' | grep -E '(cpp:|hpp:)' | wc -l
 328


- Benjamin


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


On Feb. 14, 2017, 2:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56601/
> ---
> 
> (Updated Feb. 14, 2017, 2:47 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e 
>   src/tests/master_validation_tests.cpp 
> fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
> 
> Diff: https://reviews.apache.org/r/56601/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56652: Windows: Change ZK patch to fix macro redefinition warnings on MSVC.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [56652, 56594, 56593, 56592, 56591, 56505, 56504, 55749, 
0, 55549, 55547, 55546, 55543, 55162, 55030, 55024, 55023, 55022]

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

Error:
2017-02-14 14:21:06 URL:https://reviews.apache.org/r/55547/diff/raw/ 
[11453/11453] -> "55547.patch" [1]
error: patch failed: 3rdparty/stout/include/Makefile.am:112
error: 3rdparty/stout/include/Makefile.am: patch does not apply

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

- Mesos Reviewbot


On Feb. 14, 2017, 11:01 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56652/
> ---
> 
> (Updated Feb. 14, 2017, 11:01 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, compiling against ZK on Windows will cause many macro
> redefinition errors. This occurs because ZK has a `winstdint.h` file,
> which contains definitions for macros that historically have not existed
> in the MSVC toolchain.
> 
> However, since MSVC 1900, many of these now exist. Here, we introduce a
> patch file that will condition out these definitions for versions 1900
> and later.
> 
> Additionally, the previous patch file was not fully correct, resulting
> in the patch being "fuzzed" when it was applied. This patch generates
> the correct output, allowing the patches to apply cleanly.
> 
> 
> Diffs
> -
> 
>   3rdparty/zookeeper-06d3f3f.patch be2ceaf529895c92dcf53984d6d88c78fb1d74ec 
> 
> Diff: https://reviews.apache.org/r/56652/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 56601: Used single quotes instead of backticks in error message.

[Gastón Kleiman]

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


Fix it, then Ship it!




I think that we should also update `checks/checker.cpp` and 
`checks/health_checker.cpp`:

```
$ ag --cpp --nomultiline '^\s*[^/"]+".*`'
checks/checker.cpp
60:"Check's `CommandInfo` is invalid: " + error->message);

checks/health_checker.cpp
:"Health check's `CommandInfo` is invalid: " + error->message);
```


src/master/validation.cpp (line 339)


I guess you also want to use single quotes here.



src/master/validation.cpp (line 770)


ditto



src/master/validation.cpp (line 961)


ditto



src/master/validation.cpp (line 1239)


ditto


- Gastón Kleiman


On Feb. 14, 2017, 1:47 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56601/
> ---
> 
> (Updated Feb. 14, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e 
>   src/tests/master_validation_tests.cpp 
> fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
> 
> Diff: https://reviews.apache.org/r/56601/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56646: Fixed indentations in master/http.cpp.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56644, 56645, 56646]

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 Feb. 14, 2017, 7:14 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56646/
> ---
> 
> (Updated Feb. 14, 2017, 7:14 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indentations in master/http.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp caa1fabb7a709d993ff711cc7803dd05000fa987 
>   src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
> 
> Diff: https://reviews.apache.org/r/56646/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54846: Removed `docker exec` when perform health checks in docker executor.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Dec. 18, 2016, 5:30 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54846/
> ---
> 
> (Updated Dec. 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón 
> Kleiman, and Lukas Loesche.
> 
> 
> Bugs: MESOS-4812
> https://issues.apache.org/jira/browse/MESOS-4812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> After Mesos could specify namespaces to enter to perform health checks,
> the command health check in docker executor doesn't need to wrap
> command with `docker exec` anymore. Remove `docker exec` aslo fixes
> the problem when escape quote character in command health checks.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
>   src/tests/health_check_tests.cpp 0a6d2dd295408dcc0434f3573e307e685f9abfe4 
> 
> Diff: https://reviews.apache.org/r/54846/diff/
> 
> 
> Testing
> ---
> 
> Added new test cases `HealthCheckTest.ROOT_DOCKER_DockerEnvironmentSetup` and 
> run 
> 
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest*"
> [--] Global test environment tear-down
> [==] 22 tests from 1 test case ran. (75423 ms total)
> [  PASSED  ] 22 tests.
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 56652: Windows: Change ZK patch to fix macro redefinition warnings on MSVC.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Repository: mesos


Description
---

Currently, compiling against ZK on Windows will cause many macro
redefinition errors. This occurs because ZK has a `winstdint.h` file,
which contains definitions for macros that historically have not existed
in the MSVC toolchain.

However, since MSVC 1900, many of these now exist. Here, we introduce a
patch file that will condition out these definitions for versions 1900
and later.

Additionally, the previous patch file was not fully correct, resulting
in the patch being "fuzzed" when it was applied. This patch generates
the correct output, allowing the patches to apply cleanly.


Diffs
-

  3rdparty/zookeeper-06d3f3f.patch be2ceaf529895c92dcf53984d6d88c78fb1d74ec 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 56530: Prevent offers for old agents being sent to MULTI_ROLE frameworks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56530]

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 Feb. 14, 2017, 3:48 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56530/
> ---
> 
> (Updated Feb. 14, 2017, 3:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a mixed cluster, when an old agent (not MULTI_ROLE capable)
> registers with new master (MULTI_ROLE capable), it cannot correctly
> handle tasks from a MULTI_ROLE framework. Therefore, we should
> disallow offers from these agents being sent to MULTI_ROLE frameworks.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
>   src/tests/master_tests.cpp c8ef9d6bd9f4f8d631ac87d5e5f8dbe679a70fd1 
> 
> Diff: https://reviews.apache.org/r/56530/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh 
> --gtest_filter="MasterTest.MultiRoleFrameworkDoesNotReceiveOfferFromOldAgent" 
> --gtest_break_on_failure --gtest_repeat=-1
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56376: Updated allocator test to support create multi role framework.

[Guangya Liu]

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

(Updated 二月 14, 2017, 10:40 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Updated allocator test to support create multi role framework.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
a866d03c0b7a676d08fb2fb1e321133c9f5363fc 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 56376: Updated allocator test to support create multi role framework.

[Guangya Liu]

> On 二月 13, 2017, 11:31 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 221-222
> > 
> >
> > You should probably do this only if the caller did not specify the 
> > multi role capability explicitly? Or FAIL the test if the caller specifies 
> > it with a message saying that we mandate that these tests are all using 
> > MULTI_ROLE, so no need to pass it.

OK, I will do this only if the caller did not specify the multi role capability 
explicitly, this option seems simple and also easy to be used by others when 
creating multi role frameworks.


- Guangya


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


On 二月 11, 2017, 10:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56376/
> ---
> 
> (Updated 二月 11, 2017, 10:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocator test to support create multi role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> a866d03c0b7a676d08fb2fb1e321133c9f5363fc 
> 
> Diff: https://reviews.apache.org/r/56376/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 56645: Added agent capabilities to `/state`(v0) endpoint of agent.

[Benjamin Bannier]

> On Feb. 14, 2017, 10:57 a.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, lines 1246-1247
> > 
> >
> > I see we have a type `SlaveInfo::Capability` defined, but don't store 
> > any values there. Shouldn't we have a repeated field there (similar to what 
> > we currently have for e.g., agent attributes, instead of synthetizing 
> > something on the spot here?

Per our offline discussion, not storing this information in `SlaveInfo` is 
deliberate and follows the pattern already in place for version which is also 
communicated via `RegisterSlaveMessage`s instead of configured in `SlaveInfo`. 
As hardcoding the same thing in two different places doesn't seem too resilent, 
do you think it would make sense to at least introduce a constant for agent 
capabilities that we could reference in both `slave/http`, and agent 
registration code (the version is already taken from a constant 
`MESOS_VERSION`)?


- Benjamin


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


On Feb. 14, 2017, 11:19 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56645/
> ---
> 
> (Updated Feb. 14, 2017, 11:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent capabilities to `/state`(v0) endpoint of agent.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 3d5ab59ddc4dce4d791c1b439f5e1459d1a724a4 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
>   src/master/http.cpp caa1fabb7a709d993ff711cc7803dd05000fa987 
>   src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
>   src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da 
> 
> Diff: https://reviews.apache.org/r/56645/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56645: Added agent capabilities to `/state`(v0) endpoint of agent.

[Jay Guo]

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

(Updated Feb. 14, 2017, 6:19 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


Changes
---

address part of BenB's comments.


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


Repository: mesos


Description
---

Added agent capabilities to `/state`(v0) endpoint of agent.


Diffs (updated)
-

  src/common/http.hpp 3d5ab59ddc4dce4d791c1b439f5e1459d1a724a4 
  src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
  src/master/http.cpp caa1fabb7a709d993ff711cc7803dd05000fa987 
  src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
  src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da 

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


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 56644: Added a member variable in Slave to store slave capabilities.

[Benjamin Bannier]

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



I am not sure we need this here, see 
https://reviews.apache.org/r/56645/#comment237329.

- Benjamin Bannier


On Feb. 14, 2017, 8:13 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56644/
> ---
> 
> (Updated Feb. 14, 2017, 8:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Slave should have a way to access its own set of capabilities. This
> patch introduced a member variable to store slave capabilities.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
> 
> Diff: https://reviews.apache.org/r/56644/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56645: Added agent capabilities to `/state`(v0) endpoint of agent.

[Benjamin Bannier]

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




src/common/http.hpp (lines 123 - 130)


Let's try to keep this alphabetized.



src/slave/http.cpp (lines 1246 - 1247)


I see we have a type `SlaveInfo::Capability` defined, but don't store any 
values there. Shouldn't we have a repeated field there (similar to what we 
currently have for e.g., agent attributes, instead of synthetizing something on 
the spot here?


- Benjamin Bannier


On Feb. 14, 2017, 8:13 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56645/
> ---
> 
> (Updated Feb. 14, 2017, 8:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent capabilities to `/state`(v0) endpoint of agent.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 3d5ab59ddc4dce4d791c1b439f5e1459d1a724a4 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
>   src/master/http.cpp caa1fabb7a709d993ff711cc7803dd05000fa987 
>   src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
>   src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da 
> 
> Diff: https://reviews.apache.org/r/56645/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56591: Stout: Removed MSVC compiler warnings.

[Alex Clemmer]

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

(Updated Feb. 14, 2017, 9:05 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
---

Removed unnecessary comments.


Repository: mesos


Description
---

Stout: Removed MSVC compiler warnings.


Diffs (updated)
-

  3rdparty/stout/include/stout/abort.hpp 
b7dba032f579ead83f4e04c942239e721632eeb4 
  3rdparty/stout/include/stout/duration.hpp 
0a30a09678c20937caa6f094c3c63a326e357932 
  3rdparty/stout/include/stout/gzip.hpp 
7040a3275370e7447e843c2471f35e2ba26166e4 
  3rdparty/stout/include/stout/os/windows/dup.hpp 
1fc4d55393aa617d1b67178c945ac0af91c30c99 
  3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/fd.hpp 
fddaaa54deaea5e6ef3947142870c7fef76e76aa 
  3rdparty/stout/include/stout/protobuf.hpp 
c0f03bc547cddba29309c429b34a0c1e6015c8ea 
  3rdparty/stout/include/stout/windows/os.hpp 
b5172fca96c4151f4b1ebb6d343022558f45fc34 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 56592: Libprocess: Removed MSVC compiler warnings.

[Alex Clemmer]

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

(Updated Feb. 14, 2017, 8:58 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
---

Removed more warnings.


Repository: mesos


Description
---

Libprocess: Removed MSVC compiler warnings.


Diffs (updated)
-

  3rdparty/libprocess/include/process/metrics/counter.hpp 
a13cc7e18c8b23eae83c326d63874d9d2aaedc0d 
  3rdparty/libprocess/include/process/network.hpp 
0590e7a67275b9e37af2a49c050daab5eeaee7a5 
  3rdparty/libprocess/src/encoder.hpp b019bf90c0aabbce50d90f5ed6f3fd25d725e542 

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


Testing
---


Thanks,

Alex Clemmer