Re: Review Request 55255: Added a test to check 'roles' is included in the `/state`.

[Jay Guo]

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

(Updated Jan. 16, 2017, 3:03 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Bannier, Benjamin Mahler, 
and Guangya Liu.


Changes
---

rebase


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


Repository: mesos


Description
---

Added a test to check 'roles' is included in the `/state`.


Diffs (updated)
-

  src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a 

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


Testing
---

make check

start a Mesos cluster and query `/state` to see 'roles' is included.


Thanks,

Jay Guo

Re: Review Request 55253: Added 'roles' section in the response of `/state` endpoint.

[Jay Guo]

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

(Updated Jan. 16, 2017, 2:31 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Bannier, Benjamin Mahler, 
and Guangya Liu.


Changes
---

rebase


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


Repository: mesos


Description
---

Roles are becoming a more significant semantic, especially within
multi-tenancy scenario. Users and operators may be interested in
resources used by a particular role, as well as frameworks, tasks
and executors registered with that role. Therefore, we added a new
'roles' section in `/state` endpoint to display information about
roles.


Diffs (updated)
-

  src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55254: Inserted `getRoles` to `getState`.

[Jay Guo]

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

(Updated Jan. 16, 2017, 2:31 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Bannier, Benjamin Mahler, 
and Guangya Liu.


Changes
---

rebase


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


Repository: mesos


Description
---

As we added 'roles' section in `/state` endpoint, this patch does
the same thing with v1 API.


Diffs (updated)
-

  include/mesos/master/master.proto 03203c8ddd05876339b1c56789b7bb140499d49d 
  include/mesos/v1/master/master.proto f8edf39a68752c8601cece345f52bce8b9a8a68b 
  src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
  src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55252: Refactored `roles()` and `getRoles()` to reuse common logic.

[Jay Guo]

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

(Updated Jan. 16, 2017, 2:30 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Bannier, Benjamin Mahler, 
and Guangya Liu.


Changes
---

rebase and squashed previous patch into this one to make the review more 
readable.


Summary (updated)
-

Refactored `roles()` and `getRoles()` to reuse common logic.


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


Repository: mesos


Description (updated)
---

Some common logic are extracted into two new methods `filterRoles`
and `_getRoles`, which can be reused by both v0 and v1 APIs. This
is a step toward adding 'roles' section into '/state' endpoint.


Diffs (updated)
-

  src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
  src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55554: Fixed a typo.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [4]

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

- Mesos ReviewBot


On Jan. 16, 2017, 4:06 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4/
> ---
> 
> (Updated Jan. 16, 2017, 4:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
> 
> Diff: https://reviews.apache.org/r/4/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

[Jay Guo]

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



I wonder how we deal with default role `*` in multi-role scenario? I remember 
that we require `*` to be explicitly specified for a multi-role framework, is 
that still true?


src/master/master.cpp (line 2478)


I think we don't necessarily have `framework` here if framework information 
hasn't been reconstructed from agent failover yet. See 
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2563-L2571

I feel that we need to introduce a cross-checking validation that checks 
"new" frameworkInfo against "old" one if available. And this may be done in 
`updateFrameworkInfo`?



src/master/master.cpp (lines 2491 - 2492)


Note that you could use `framework.capabilities` here since 
https://reviews.apache.org/r/55021/ is landed.


- Jay Guo


On Jan. 12, 2017, 11:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> ---
> 
> (Updated Jan. 12, 2017, 11:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
> https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> ---
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55381: Added test for framework upgrading to multi-role capability.

[Jay Guo]

> On Jan. 16, 2017, 12:37 p.m., Jay Guo wrote:
> > src/tests/master_validation_tests.cpp, line 2559
> > 
> >
> > I think this test is not valid since we currently don't have logic to 
> > check framework upgrade yet.
> > 
> > This test passes because `role` update is ignored with a warning 
> > generated here: 
> > https://github.com/apache/mesos/blob/master/src/master/master.hpp#L2488-L2491
> > 
> > `roles` field is actually not checked against old `role`.

Just saw next patch in the review chain. Could you reverse the order since this 
test should actually goes after the framework upgrade patch.


- Jay


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


On Jan. 12, 2017, 11:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> ---
> 
> (Updated Jan. 12, 2017, 11:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55381/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55381: Added test for framework upgrading to multi-role capability.

[Jay Guo]

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




src/tests/master_validation_tests.cpp (line 2559)


I think this test is not valid since we currently don't have logic to check 
framework upgrade yet.

This test passes because `role` update is ignored with a warning generated 
here: 
https://github.com/apache/mesos/blob/master/src/master/master.hpp#L2488-L2491

`roles` field is actually not checked against old `role`.


- Jay Guo


On Jan. 12, 2017, 11:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> ---
> 
> (Updated Jan. 12, 2017, 11:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55381/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 55554: Fixed a typo.

[Jay Guo]

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

Review request for mesos, Benjamin Mahler and Guangya Liu.


Repository: mesos


Description
---

Fixed a typo.


Diffs
-

  src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55548: Used `os::environment` instead of `os::raw::environment` in agent.

[Alex Clemmer]

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

(Updated Jan. 16, 2017, 2:25 a.m.)


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


Changes
---

Fix build for AC Linux.


Repository: mesos


Description
---

This is necessary as a follow-up to r/5547, which removes
`os::raw::environment` from Stout's Windows API.


Diffs (updated)
-

  src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Jay Guo]

> On Jan. 13, 2017, 4:17 p.m., Jay Guo wrote:
> > When a resource with `*` is offered to a multi-role framework, how does the 
> > framework decide which role to reserve the resource for?
> 
> Benjamin Bannier wrote:
> Frameworks with default role cannot reserve resources; this is the first 
> check in the validation function being updated in this patch.

For example. My framework has role `foo`, `bar` and `*`. I get an offer with 
resource of role `*`. I want to reserve it for `foo`. Previously I simply issue 
`RESERVE` operation and the resource will be reserved for the ONLY role I have. 
But in multi-role scenario, how should I tell which role I want to reserve the 
resource for?


- Jay


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


On Jan. 13, 2017, 11:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> ---
> 
> (Updated Jan. 13, 2017, 11:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Jay Guo]

> On Jan. 13, 2017, 4:15 p.m., Jay Guo wrote:
> > src/master/master.cpp, lines 3944-3950
> > 
> >
> > I think we explicitly disallow empyt role field? 
> > https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
> > Also, should the case `framework->info.has_role() == false && 
> > framework->info.role() != "*"` be invalid and caught upon subscription?
> 
> Benjamin Bannier wrote:
> Multi-role frameworks would use the `roles` field, in which case the 
> `role` field would not be set. We need to distinguish here whether a 
> framework uses `role` or `roles`. The check can be simplified to 
> `framework->info.role() != "*"` though, independently of what we assume about 
> this field upstream. I made that change.
> 
> Does that address your comment?

then why not using framework capability here? note that 
https://reviews.apache.org/r/55021/ is landed which you may want to use.


- Jay


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


On Jan. 13, 2017, 11:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> ---
> 
> (Updated Jan. 13, 2017, 11:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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 Jan. 15, 2017, 9:44 p.m.)


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


Changes
---

Updated typos I made copying from my Windows box. :(


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 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 
  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 30735e28a26ff713469711d63538676ed4e327d9 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55550: Windows: Enabled health checker tests.

[Alex Clemmer]

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

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


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


Repository: mesos


Description
---

Windows: Enabled health checker tests.


Diffs
-

  src/tests/health_check_tests.cpp 0a6d2dd295408dcc0434f3573e307e685f9abfe4 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55549: Windows: Added health checker to build.

[Alex Clemmer]

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

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


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


Repository: mesos


Description
---

This is a blocker for MESOS-6709, which requires both the health checker
and the TCP connector to build and work on Windows.


Diffs
-

  src/health-check/CMakeLists.txt ce195e2820c70dd7ebec1f06a6382f58fc729af7 
  src/health-check/health_checker.cpp a8424b75927d15dc1b897faf0e47cf075c70ff26 
  src/health-check/tcp_connect.cpp ad1e932da53c8f6b0ae77dfa6b6bb3d642273af9 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55548: Used `os::environment` instead of `os::raw::environment` in agent.

[Alex Clemmer]

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

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


Repository: mesos


Description
---

This is necessary as a follow-up to r/5547, which removes
`os::raw::environment` from Stout's Windows API.


Diffs
-

  src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135 

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


Testing
---


Thanks,

Alex Clemmer

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

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 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 
  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 30735e28a26ff713469711d63538676ed4e327d9 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55546: Added platform-independent constants `DEV_NULL` and `TRUE_COMMAND`.

[Alex Clemmer]

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

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


Repository: mesos


Description
---

Added platform-independent constants `DEV_NULL` and `TRUE_COMMAND`.


Diffs
-

  3rdparty/stout/include/stout/gtest.hpp 
a004a378cb467495234d77a0c56fbea6e7bec420 
  3rdparty/stout/include/stout/os/constants.hpp 
c71d52e1bc0433f9922f26a6eb486235bb9880d4 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Alex Clemmer]

> On Dec. 24, 2016, 10:06 p.m., Daniel Pravat wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1611
> > 
> >
> > This code was working before. you should not change it to make the 
> > taest work and add overhead in production.

Per Slack discussion, this is necessary. We should be launching this binary out 
of the launcher directory to match POSIX semantics.


- Alex


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


On Jan. 15, 2017, 10:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Jan. 15, 2017, 10:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 10:46 a.m.)


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


Changes
---

Address Daniel's comments.


Repository: mesos


Description
---

Currently in `MesosContainerizerProcess::_launch`, we are passing a
malformatted shell command to the launcher. This causes the
containerizer process to crash immediately upon invocation in all
executor tests.

This commit will fix this command.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55022: Windows: Cause errors to be correctly reported in `io::read`.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 10:23 a.m.)


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


Changes
---

Rebased, addressed Joseph's comments.


Repository: mesos


Description
---

Windows: Cause errors to be correctly reported in `io::read`.


Diffs (updated)
-

  3rdparty/libprocess/src/io.cpp c37ec1811fab7d8d33f0d5fd8703ab121d6db514 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 10:17 a.m.)


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


Changes
---

Add headers, add qualified namespaces.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
  src/slave/containerizer/mesos/utils.hpp 
a54106dc4893bb222f42ede936ac9029e817faf9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55490: Used the `mesos/mesos-tidy` image from DockerHub.

[Benjamin Bannier]

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


Fix it, then Ship it!




This is great. Could you make sure to follow up with setting `mesos/mesos-tidy` 
up for automated builds? Before building the image in the dockerhub time 
constraints was hard, but I am optimistic this would work now with the changes 
from https://reviews.apache.org/r/55488/.


support/mesos-tidy.sh (line 39)


Removing the image sure made sense for an always regenerated image, but 
does it also for a relatively static image? I as a developer would prefer to 
not always have to repull the image, but maybe e.g., a CI has different 
constraints.

Let's keep the `docker pull` to ensure a recent enough image, but let's 
remove the `docker rmi`.


- Benjamin Bannier


On Jan. 14, 2017, 9:02 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> ---
> 
> (Updated Jan. 14, 2017, 9:02 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used the `mesos/mesos-tidy` image from DockerHub.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 
> 
> Diff: https://reviews.apache.org/r/55490/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55489: Used CMake to generate the compilation database instead.

[Benjamin Bannier]

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


Ship it!




- Benjamin Bannier


On Jan. 14, 2017, 9:01 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55489/
> ---
> 
> (Updated Jan. 14, 2017, 9:01 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used CMake to generate the compilation database instead.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7 
>   support/mesos-tidy/Dockerfile 82da813f78ccfe17812e09647cb7b3db4e279464 
>   support/mesos-tidy/entrypoint.sh 72872375f3e5ad19bc75949f9e3db14d6068f9b6 
> 
> Diff: https://reviews.apache.org/r/55489/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55544: Windows: Set `MAXHOSTNAMELEN` to an appropriate value.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [55544, 55543, 55328, 55327, 55314, 55313, 55312, 55311, 
55162, 55161, 55040, 55039, 55038, 55037, 55030, 55024, 55023, 55022]

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

Error:
2017-01-15 09:41:00 URL:https://reviews.apache.org/r/55022/diff/raw/ 
[1047/1047] -> "55022.patch" [1]
error: patch failed: 3rdparty/libprocess/src/io.cpp:79
error: 3rdparty/libprocess/src/io.cpp: patch does not apply

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

- Mesos ReviewBot


On Jan. 15, 2017, 9:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55544/
> ---
> 
> (Updated Jan. 15, 2017, 9:12 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-3398
> https://issues.apache.org/jira/browse/MESOS-3398
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The POSIX standard defines a function `getnameinfo`, but does not
> specify a maximum length that a hostname can have. To simplify the logic
> of choosing a reasonable buffer size to hold the hostname, most
> implementations of this specification expose an additional symbol (e.g.,
> NI_MAXHOST) that represents the maximum length a hostname can have.
> 
> Our code uses `MAXHOSTNAMELEN`, and so on platforms where this symbol is
> not implemented (such as Windows), it is necessary to define it.
> 
> Currently, `stout/windows.hpp` defines this to be `64`. This value was
> chosen because other popular codebases seemed to use this value. But, as
> MESOS-3398 explains, we were never sure that this was correct.
> 
> Now we have compelling evidence that it isn't: the Windows documentation
> for `getnameinfo` clearly states that the maximum length of a hostname
> must is captured as `NI_MAXHOST`, and in all recent versions of the
> standard libraries, the value of this symbol is 1025.
> 
> Hence, for machines with a hostname longer than 64 bytes, almost all the
> tests will abort mysteriously, with a cryptic error that claims that the
> "data area passed to a system call is too small", which essentially
> means that this particular buffer is too small to hold the hostname.
> 
> This commit will cause this value to be correctly set on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows.hpp 
> d89c70902cf6051608c2cb290b0727cbb45c 
> 
> Diff: https://reviews.apache.org/r/55544/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 9:22 a.m.)


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


Changes
---

Moved `defaultPath` location around, again.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
  src/slave/containerizer/mesos/utils.hpp 
a54106dc4893bb222f42ede936ac9029e817faf9 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55543: Fail the build if %PreferredToolArchitecture% is not set to `x64`.

[Alex Clemmer]

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

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


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


Repository: mesos


Description
---

Before building Mesos on a Windows machine, it is necessary to set
`%PreferredToolArchitecture%` to the value `x64`. This is necessary to
work around (at least) two bugs in the MSVC backend: in particular, the
linker can sometimes take hours or days to link `mesos-x.x.x.lib`, and
the build system occasionally finds it self spuriously unable to find
file `mesos-x.x.x.lib` to link against.

These issues are well-known and documented (e.g., in the official Mesos
"getting started" document), but it is better to simply refuse to build
Mesos at all on Windows unless that environment variable is set.

This commit will introduce such a check.


Diffs
-

  cmake/CompilationConfigure.cmake 560935b81603dc58c167918d36e2ae0a4060673d 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55544: Windows: Set `MAXHOSTNAMELEN` to an appropriate value.

[Alex Clemmer]

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

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


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


Repository: mesos


Description
---

The POSIX standard defines a function `getnameinfo`, but does not
specify a maximum length that a hostname can have. To simplify the logic
of choosing a reasonable buffer size to hold the hostname, most
implementations of this specification expose an additional symbol (e.g.,
NI_MAXHOST) that represents the maximum length a hostname can have.

Our code uses `MAXHOSTNAMELEN`, and so on platforms where this symbol is
not implemented (such as Windows), it is necessary to define it.

Currently, `stout/windows.hpp` defines this to be `64`. This value was
chosen because other popular codebases seemed to use this value. But, as
MESOS-3398 explains, we were never sure that this was correct.

Now we have compelling evidence that it isn't: the Windows documentation
for `getnameinfo` clearly states that the maximum length of a hostname
must is captured as `NI_MAXHOST`, and in all recent versions of the
standard libraries, the value of this symbol is 1025.

Hence, for machines with a hostname longer than 64 bytes, almost all the
tests will abort mysteriously, with a cryptic error that claims that the
"data area passed to a system call is too small", which essentially
means that this particular buffer is too small to hold the hostname.

This commit will cause this value to be correctly set on Windows.


Diffs
-

  3rdparty/stout/include/stout/windows.hpp 
d89c70902cf6051608c2cb290b0727cbb45c 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 9:02 a.m.)


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


Changes
---

Fix typos in commit message.


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


Repository: mesos


Description (updated)
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
9fcad933e1ec3b52d4dc366ba80d282deea04c0b 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 9 a.m.)


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


Changes
---

Address Till's comments.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `default_path` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
9fcad933e1ec3b52d4dc366ba80d282deea04c0b 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

> On Dec. 27, 2016, 2:12 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 148
> > 
> >
> > By not adding `syswow64` we are excluding 32bit runnables, is this 
> > intentional and documented? Is this still a thing on windows?
> 
> Joseph Wu wrote:
> Actually, in the current implementation of subprocess on Windows, any 
> user-specified environment variables that conflict with system environment 
> variables will be overridden at subprocess launch.  This means you can 
> specify a `PATH=nowhere` and we'll just ignore it.
> 
> At some point, we may loosen this behavior, as we only did this because 
> the test coverage of the Windows code at the time was nil, and a bunch of 
> other changes kept breaking the environment variables on Windows :)

Actually, the issue is that I'm not sure how to get that folder name without 
depending on the Shell API. (Not a Windows expert, let me know if you have a 
good way to do this.) We are very careful not to depend on the Shell because 
it's not available on Server Nano.

I'll drop it for now, but I'd be quite happy to add this if there was a 
suggestion for doing it that worked around this constraint.


- Alex


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


On Dec. 26, 2016, 9:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Dec. 26, 2016, 9:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `default_path` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55030: CMake: Added source groups for libprocess build.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 8:47 a.m.)


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


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


Repository: mesos


Description
---

Currently in IDEs such as XCode and Visual Studio, all mesos source
files are grouped into one huge folder, with no attempt made to reflect
the hierarchy of folders that exist on disk.

This commit groups libprocess files into relevant source groups so that
they appear in a sensible directory structure in these IDEs.

In addition to beginning to directly address MESOS-3542, this is also
(indirectly) the first step towards addressing the problem of breaking
libmesos up into smaller binaries (MESOS-3542).


Diffs (updated)
-

  3rdparty/libprocess/cmake/ProcessConfigure.cmake 
873f41d844faa0d442f77411e94314a89be5f046 
  3rdparty/libprocess/src/CMakeLists.txt 
60f0e76dfd237d9a12a366b413802d1a96892b55 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
0b2660cb16f5d8d8dc66e6995061d0b832182351 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 8:46 a.m.)


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


Repository: mesos


Description
---

Currently libprocess will attempt to use sockets without initializing
the socket stack on Windows. This commit will resolving this problem by
causing `process::initialize` to initialize the socket stack.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
  3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

> On Jan. 11, 2017, 2:19 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1046
> > 
> >
> > s/teard down/teardown/
> > 
> > `process::finalize` should probably perform the socket teardown, or at 
> > least give the option to do so.  
> > 
> > Currently, very few of our processes call `process::finalize`.  They 
> > usually just rely on the OS cleaning up after them.
> 
> Alex Clemmer wrote:
> I'm pretty comfortable allowing `process::finalize` to clean up after 
> itself, especially resources that it owns and abstracts away, but disposing 
> of something that libprocess _doesn't_ own, and especially something as 
> global and critical as the socket stack, seems a bit out of scope.
> 
> I do think it's prudent to entertain proposals about having it 
> _optionally_ dispose of the socket stack, but I can't think of an obviously 
> good way to do this. A default parameter in `process::finalize` would only be 
> meaningful semantically on Windows, no?

Per Slack conversation, we've decided to add an optional parameter to perform 
WSA cleanup.


- Alex


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


On Dec. 24, 2016, 10:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Dec. 24, 2016, 10:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 889a03444eaee7b5ad2be65bb414c30062d4a4f0 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55327: Windows: Fixed hanging symlink bug in `os::rmdir`.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 8:40 a.m.)


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


Changes
---

Fix misspelling.


Repository: mesos


Description
---

The Windows implementation of `os::rmdir` will fail to delete "hanging"
symlinks (i.e., symlinks whose targets do not exist). Note that on
Windows this bug is specific to symlinks whose targets are _deleted_,
since it is impossible to create a symlink whose target does not exist.

The primary issue that causes this problem is that it is very difficult
to tell whether a symlink points at a directory or a file unless you
resolve the symlink and determine whether the target is a directory or a
file. In situations where the target does not exist, we can't use this
information, and so `os::rmdir` occasionally mis-routes a symlink to
(what was) a directory to a `::remove` call, which will fail with a
cryptic error.

To fix this behavior, this commit will introduce code that simply tries
to remove the reparse point with both `RemoveDirectory` and
`DeleteFile`, and if either succeeds, we report success for the
operation. This represents a "best effort"; in the case that the reparse
point represents something more exotic than a symlink, we will still
fail, but by choosing not to verify whether the target is a directory or
a file, we simplify the code and still obtain the outcome of having
deleted the directory.

This commit is the primary blocker for MESOS-6707, as deleting the Agent
sandbox will sometimes cause us to delete the latest run directory for
the executor before the symlinked `latest` directory itself. This causes
the delete to fail, and then the GC tests to fail, since they tend to
assert the directory does not exist.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/windows/rmdir.hpp 
4437484c068e9ef046e0be14683c97db447f2da1 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
988d41b7fdd11cc96ce005671a7c62d1b5a3615d 

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


Testing
---


Thanks,

Alex Clemmer