Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 7, 2016, 10:51 p.m., Jie Yu wrote:
> > Looks like the review bot has a segfault?

The failed test 'HTTPCommandExecutorTest.TerminateWithACK' is broken on master 
head so it's not it. I saw Anand pinging Qian on the build mailing list. We can 
have a JIRA to track it.


- Jiang Yan


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


On Nov. 7, 2016, 12:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 7, 2016, 12:13 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jie Yu]

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



Looks like the review bot has a segfault?

- Jie Yu


On Nov. 7, 2016, 8:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 7, 2016, 8:13 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jie Yu]

> On Nov. 5, 2016, 9:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1405
> > 
> >
> > I'll be nice to use environment variable for all flags here. I think we 
> > probably need a helper in FlagsBase to return the corresponding environment 
> > variables for the flags:
> > ```
> > // Export flags as environment variables.
> > map FlagsBase::environment(
> > const Option& prefix);
> > ```
> > 
> > And then, we can just merge with os::environment and pass it to 
> > Launcher::fork
> 
> Jiang Yan Xu wrote:
> This helper looks like handy. 
> 
> I am not necessarily against this but wanted us to give it more thought. 
> 
> 1. Should all Mesos libexec binaries work this way?
> 2. Removing all of the flags makes all the helper binary instances look 
> the same through `ps`. Yes privileged users can still get the info in /proc 
> for individual processes but we loose  we lose easy "grep-pability" for 
> debugging. 
> 
> To me the line to draw is between user-supplied info (prefer env vars) 
> and Mesos generated info (e.g., runtime dir which encodes contianerId) which 
> is not sensitive. Thoughts?

This is just a suggestion. That's the reason I didn't put an issue there.

For 1, yes, I would say so because libexec binaries are Mesos details, which 
should not be exposed to users
For 2, one can still log that in the agent log.

But we can do that later, we don't have to do it now.


- Jie


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


On Nov. 7, 2016, 8:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 7, 2016, 8:13 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53473: Add new param user to logrotate's prepare function.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52308, 52310, 53473]

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 Nov. 7, 2016, 10:56 p.m., Sivaram Kannan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53473/
> ---
> 
> (Updated Nov. 7, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new param user to logrotate's prepare function.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> 939974736f9eb744c83036e074718d2a1eba8b0a 
>   src/slave/container_loggers/lib_logrotate.hpp 
> 28fdf3bdcc66d473521b377f66ab0b48f6900f58 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 53698d339f0f4d2dc916b53239ca0c36bbebcd42 
>   src/slave/container_loggers/logrotate.hpp 
> d1db69236f5a9b1dbb3113ad02218a512afdb46b 
>   src/slave/container_loggers/sandbox.hpp 
> e0aeb32a9ec83af049af8a10010b819c1d8b25d8 
>   src/slave/container_loggers/sandbox.cpp 
> cc263ebef7e0c3e778fabafa49faa6dd315adc45 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/tests/container_logger_tests.cpp 
> 1bb94a8461e481983f25a44737e4011ed5fc4b1f 
> 
> Diff: https://reviews.apache.org/r/53473/diff/
> 
> 
> Testing
> ---
> 
> Run the mesos-logrotate-logger with un-priviledged user and verify whether 
> the logs are getting rotated.
> Run the mesos-logrotate-logger as root user and verify whether the logs are 
> getting rotated.
> 
> 
> Thanks,
> 
> Sivaram Kannan
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated Nov. 8, 2016, 5:50 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
edd5cea8996d7c616cf9428f0f1c05d70c37c307 
  src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
  src/tests/resources_utils.cpp be1feb97be552af582dbba3a54fa5fa0714b3eb2 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Nov. 8, 2016, 5:50 a.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

[Anindya Sinha]

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

(Updated Nov. 8, 2016, 5:50 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased


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


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, line 129
> > 
> >
> > Keep the name?
> > 
> > Shared persistent volume is a kind of persistent volume.

Yes, agreed. This happened since to start off, we had a separate test framework 
for shared volumes, and later decided to merge the test frameworks for shared 
and regular persistent volumes into a single test.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 188-189
> > 
> >
> > Why is "shared-only" a possible value? A shard has only one persistent 
> > volume so it's either "shared" or not. How about just a `bool isShared`. 
> > (As suggested below, we can put it in the volume).

The original thinking for `flags.mode` was not scoped to a shard but to the 
framework, i.e. if the framework launches shards with tasks where every shard 
used a shared volume (`shared-only` case) or alternates between shared and 
regular volumes (`mixed` case). However, I think I liked separating that 
intention with `num_shards` (for regular volumes) and `num_shared_shards` (for 
shared volumes). So, removed `flags.mode` and added `flags.num_shared_shards`.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 215-222
> > 
> >
> > No need to act differently based on the shard type.
> > 
> > We can just have 
> > 
> > - The first task (`launched == 0`) writes to the file: `echo hello > 
> > volume/persisted`
> > - Later tasks read from the file: `cat volume/persisted`
> > 
> > For regular persistent volumes the two tasks will be sequential, for 
> > shared ones they'll be simulatenous. The fact that 1st task could finish 
> > before the 2nd is launched is not important: the example framework mainly 
> > serves the purpose of demostrating the usage of the feature and we don't 
> > want the tasks to block the CI for too long.
> > 
> > As a potentially followup we can take a read/consumer command and a 
> > write/producer command from flags (with default values). So CI would use 
> > the default values to complete quickly while a human could try out 
> > different write and reads commands which could represent more 
> > interesting/edge cases.

I think the issue is when the writer task has not done an actual write yet, but 
the reader task has done a read already (this is possible since these are 2 
separate tasks). This would not happen in shards using regular volumes (since 
we guarantee the order of task launches and hence order of read/write), but can 
happen in rare case in shared volumes. So, this is what I did to avoid that 
race:

Writer task:
`echo hello > volume/persisted` for both regular and shared volumes.

Reader task(s):
Regular volumes: `cat volume/persisted`
Shared volumes: We try for 5 iterations to read `volume/persisted` and we exit 
when we read successfully (exits with success) or we fail to read in 5 
iterations (exits with a failure).
```COUNTER=0; while [ $COUNTER -lt 5 ]; 
   do cat volume/persisted;
   if [ $? -eq 0 ]; then
 exit 0;
   fi;
   COUNTER=$[COUNTER+1]; sleep 1; done; exit 1");```


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 208-209
> > 
> >
> > Any need for `string task_id`? If so it needs to be named `taskId` but 
> > I don't see the need? Once it's assigned to the task we can get it out by 
> > `task.task_id()`.

I was doing a `echo` on the `task id` so I used a temporary variable, but I 
agree we can certainly avoid it.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 470-476
> > 
> >
> > I think it's sufficient to have the following states. (We should use a 
> > minimun number of state, we can always add more when more features are 
> > added to persistent volumes which demand more state but starting with a 
> > large number of states makes it difficult to evolve the test).
> > 
> > ```
> >   STAGING = 0, // The shard is awaiting offers to launch more tasks.
> >   RUNNING, // The shard is fully running (all its tasks are 
> > launched).
> >   TERMINATING, // The shard is terminating and needs to clean up 
> > its persistent volume.
> >   DONE // The shard is terminated.
> > ```
> > 
> > This translates to:
> > 
> > ```
> >   STAGING = 0, // In resourceOffers: launch tasks
> >   RUNNING, // In resourceOffers: noop; In 

Re: Review Request 53486: Introduced a streaming request decoder in libprocess.

[Benjamin Mahler]

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



Let's include some tests alongside the new code, like we did on the response 
side:
https://github.com/apache/mesos/commit/6ac8eb1c524dc4adbc09d20dcb8e4e31d60eeb56

Otherwise, the implementation looks good, but I would wait for the tests and 
gzip::Decompressor integration before shipping.

Higher level thought, the implementations of request vs response decoding at a 
glance look exactly the same, is there an implementation difference? Have you 
explored merging them? E.g.

```
namespace internal {
template  // We can later restrict which T's are allowed.
class Decoder
{
  ...
};
}

// Users only use these non-templated versions:
using RequestDecoder = internal::Decoder;
using ResponseDecoder = internal::Decoder;
```

- Benjamin Mahler


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53486/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This would become the default de facto decoder used by libprocess
> and replace the existing `DataDecoder`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 76dca0b272af8591880ef220ec2dc006906fbc36 
> 
> Diff: https://reviews.apache.org/r/53486/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53485: Introduced a `io::read()` overload for reading from piped reader.

[Benjamin Mahler]

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



Per our discussion with benh offline, let's move this to be a 
`Pipe::Reader::readAll` member function.

- Benjamin Mahler


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53485/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The helper reads from the pipe till EOF. This is used later to
> read BODY requests from the streaming request decoder.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> eec5efd7e6b71a783f2bb40826054d0488cee71f 
>   3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 
> 
> Diff: https://reviews.apache.org/r/53485/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53366: Introduced a streaming gzip::Decompressor.

[Benjamin Mahler]

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

(Updated Nov. 8, 2016, 4:22 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This enables incremental decompression of compressed input.


Diffs (updated)
-

  3rdparty/stout/include/stout/gzip.hpp 
b78a8a31204ee585f8e4a88eaefe7346faa46b8d 
  3rdparty/stout/tests/gzip_tests.cpp 814fb99da193cf950ba48817824acdabda2c2dfc 

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


Testing
---

Wrote a test for decompressing a byte at a time. Also ran this test manually 
with random chunk sizes.


Thanks,

Benjamin Mahler

Re: Review Request 53365: Fixed an issue in the gzip error handling.

[Benjamin Mahler]

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

(Updated Nov. 8, 2016, 4:22 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Added additionally needed includes.


Repository: mesos


Description (updated)
---

It turns out that zlib does not always set the `msg` field, the
calling code is expected to handle the case where `msg` is NULL.
I discovered this while I was playing with the library during
the implementation of a streaming decompressor.


Diffs (updated)
-

  3rdparty/stout/include/stout/gzip.hpp 
b78a8a31204ee585f8e4a88eaefe7346faa46b8d 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 53365: Fixed an issue in the gzip error handling.

[Benjamin Mahler]

> On Nov. 6, 2016, 5:47 p.m., Anand Mazumdar wrote:
> > hmm, I couldn't find any documentation around when `msg` may be not be set 
> > for error cases except for `deflateEnd()` and `inflateEnd()`. Can you share 
> > some? 
> > AFAICT, we only need to add a fallback for these 2 calls and not all of the 
> > ones as has been done in this review. Also, I don't think we need to even 
> > check the return type of `deflateEnd()`/`inflateEnd()` since we are sure 
> > that the state of the stream is consistent i.e., `Z_STREAM_END`?

Per our offline discussion, it appears that zlib does not set `msg` in many 
cases and other libraries work around this, e.g.
https://github.com/python/cpython/blob/c30098c8c6014f3340a369a31df9c74bdbacc269/Modules/zlibmodule.c#L59-L86

I've updated this patch to introduce an `internal::GzipError` to simply the 
error handling logic.
I've also updated to `ABORT` for any programming errors, that is 
(in|de)flateInit2 / (in|de)flateEnd failures, since these have to be run inside 
the Decompressor and Compressor (in the future) constructors and destructors.


- Benjamin


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


On Nov. 8, 2016, 4:17 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53365/
> ---
> 
> (Updated Nov. 8, 2016, 4:17 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It turns out that zlib does not always set the `msg` field, the
> calling code is expected to handle the case where `msg` is NULL.
> I discovered this while I was playing with the library during
> the implementation of a streaming decompressor.
> 
> I've introduced an `internal::GzipError` to simplify the error
> handling, and I've updated the code to `ABORT` in the case of
> our own programming errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gzip.hpp 
> b78a8a31204ee585f8e4a88eaefe7346faa46b8d 
> 
> Diff: https://reviews.apache.org/r/53365/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 53365: Fixed an issue in the gzip error handling.

[Benjamin Mahler]

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

(Updated Nov. 8, 2016, 4:17 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Updated to include an `internal::GzipError` to clean up the error handling. 
Also updated to `ABORT` in the case of programming errors.


Repository: mesos


Description (updated)
---

It turns out that zlib does not always set the `msg` field, the
calling code is expected to handle the case where `msg` is NULL.
I discovered this while I was playing with the library during
the implementation of a streaming decompressor.

I've introduced an `internal::GzipError` to simplify the error
handling, and I've updated the code to `ABORT` in the case of
our own programming errors.


Diffs (updated)
-

  3rdparty/stout/include/stout/gzip.hpp 
b78a8a31204ee585f8e4a88eaefe7346faa46b8d 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 52735: Removed TODO message for docker killing.

[Guangya Liu]

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



I would suggest that you create a patch chain based on the following order:

https://reviews.apache.org/r/52735/
https://reviews.apache.org/r/50128/
https://reviews.apache.org/r/50947/
https://reviews.apache.org/r/53532/
https://reviews.apache.org/r/50599/
https://reviews.apache.org/r/50125/
https://reviews.apache.org/r/53533/
https://reviews.apache.org/r/50127/

- Guangya Liu


On 十一月 7, 2016, 1:53 p.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52735/
> ---
> 
> (Updated 十一月 7, 2016, 1:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed TODO message for docker killing.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
> 
> Diff: https://reviews.apache.org/r/52735/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Joseph Wu]

> On Nov. 7, 2016, 4:38 p.m., Qian Zhang wrote:
> > I do not quite understand the intention of this patch. The description of 
> > this patch says that `Previously, the headers did not contain includes for 
> > any of the classes used in the file.`, but actually `stout/os.hpp` has been 
> > already included which almost has all the needed headers, like 
> > `stout/nothing.hpp`, `stout/try.hpp`, etc. So I guess the intention of this 
> > patch is to remove the inclusion of `stout/os.hpp` and instead include each 
> > needed headers directly, right? But I still see `stout/os.hpp` in both 
> > `utils.hpp` and `utils.cpp` in this patch which makes me a bit confused.
> 
> Joseph Wu wrote:
> The style for our include headers is to include all dependencies, so that 
> changing the include headers of a dependency does not break any upstream code.
> 
> i.e. 
> foo.hpp:
> ```
> #include 
> 
> typedef std::string Fooo;
> ```
> 
> bar.hpp:
> ```
> #include 
> #include  // <- Need this because we use it in this file.
> 
> #include "foo.hpp"
> 
> std::map bar;
> ```
> 
> The exception is any headers that are effectively aliases to other 
> headers, like `stout/lambda.hpp` (to various `` types) or 
> `src/messages/messages.hpp` (to the protobuf `messages.pb.h`).
> 
> Qian Zhang wrote:
> Thanks for the explanation! So if a header is needed by `utils.cpp`, then 
> `utils.cpp` should always include it even it has already been included in 
> `utils.hpp`.
> 
> And I have a further comment: in your updated patch, I see in 
> `utils.cpp`, you include `stout/os.hpp` and also `stout/try.hpp`, 
> `stout/nothing.hpp`, etc. which are actually already included in 
> `stout/os.hpp`. I think that is not what we are doing in Mesos, if we have 
> included `stout/os.hpp` in a .cpp, we do not need to include the header files 
> in it again. See the following code as examples:
> 
> https://github.com/apache/mesos/blob/1.0.1/src/slave/containerizer/mesos/provisioner/backend.cpp#L19
> 
> https://github.com/apache/mesos/blob/1.0.1/src/slave/containerizer/docker.cpp#L38
> 
> Both of the above two files use `Try` and `Nothing`, but they only 
> include `stout/os.hpp` not `stout/try.hpp` and `stout/nothing.hpp`.

We're a bit inconsistent about this.  Maybe once we have the whole clang-tidy 
thing setup, we can define some rules about this.

Almost every file includes `Try` and `Option` and `Nothing` (and a few other 
common things).  We have a few hundred files which include those headers 
directly... and a few hundred that do not.

In this patch, my thought process is, "if I'm correcting the headers, I might 
as well add all the appropriate ones".


- Joseph


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


On Nov. 7, 2016, 4:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53556/
> ---
> 
> (Updated Nov. 7, 2016, 4:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the headers in the provisioner utility.  Previously, the
> headers did not contain includes for any of the classes used in the
> file.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/utils.hpp 
> 4efce0ed67312417495af8c884ce26ef6affaccb 
>   src/slave/containerizer/mesos/provisioner/utils.cpp 
> 340cf48819679cdd8d08d76dcd2328234477a15f 
> 
> Diff: https://reviews.apache.org/r/53556/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53509: Changed mesos-execute --master flag parsing.

[Armand Grillet]

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

(Updated Nov. 8, 2016, 2:01 a.m.)


Review request for mesos, Joseph Wu and Till Toenshoff.


Summary (updated)
-

Changed mesos-execute --master flag parsing.


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


Repository: mesos


Description (updated)
---

This commit makes the --master argument required.
The parsing of the argument now happens in src/master/detector.cpp.


Diffs
-

  src/cli/execute.cpp b265bc6390ed8329a200408ef45512f900f9b999 

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


Testing
---

Testing done by running a master with `--zk=zk://localhost:2181/mesos` then an 
agent with `--master=zk://localhost:2181/mesos` and then executing a sleep task 
on Mesos with `--master=zk://localhost:2181/mesos`.


Thanks,

Armand Grillet

Review Request 53560: LOG(FATAL) transformed into EXIT(EXIT_FAILURE) in process::initialize.

[Armand Grillet]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

We now do an `EXIT(EXIT_FAILURE)` instead of `LOG(FATAL)` when
initialising a process.


Diffs
-

  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 

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


Testing
---

make check (macOS)


Thanks,

Armand Grillet

Re: Review Request 53561: Stopped taking task_id in scheduler::SendAcknowledge action in tests.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 8, 2016, 1:51 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53561/
> ---
> 
> (Updated Nov. 8, 2016, 1:51 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Taking task_id is not necessary because the status update itself
> (arg1) already has task_id in it.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 
> 
> Diff: https://reviews.apache.org/r/53561/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 53559: Transformed env variable parsing into Flags in process.cpp.

[Armand Grillet]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Environment variables LIBPROCESS_ + (IP, ADVERTISE_IP,PORT,
ADVERTISE_PORT) are now in a Flags object. Heavily inspired
by Joseph's review request [43261](https://reviews.apache.org/r/43261/).


Diffs
-

  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 

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


Testing
---

make check (macOS)


Thanks,

Armand Grillet

Re: Review Request 53561: Stopped taking task_id in scheduler::SendAcknowledge action in tests.

[Jie Yu]

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

(Updated Nov. 8, 2016, 1:51 a.m.)


Review request for mesos.


Repository: mesos


Description
---

Taking task_id is not necessary because the status update itself
(arg1) already has task_id in it.


Diffs
-

  src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 

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


Testing (updated)
---

make check


Thanks,

Jie Yu

Re: Review Request 53466: Added test to verify container status for nested containers.

[Jie Yu]

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

(Updated Nov. 8, 2016, 1:51 a.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


Bugs: MESOS-6465 and MESOS-6528
https://issues.apache.org/jira/browse/MESOS-6465
https://issues.apache.org/jira/browse/MESOS-6528


Repository: mesos


Description
---

This patch is used to verify that container status is properly set in
task status update for nested containers.


Diffs
-

  src/tests/default_executor_tests.cpp bb954fc48717b2c02abe1c98753b009ec3bb695a 
  src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 53313: Windows: Disable persistent state for Windows master.

[Joseph Wu]

> On Oct. 31, 2016, 2:20 p.m., Joseph Wu wrote:
> > src/local/local.cpp, lines 214-228
> > 
> >
> > If you move `#ifdef` up one, you won't need to add the extra `EXIT(..)`.
> 
> Alex Clemmer wrote:
> I personally would rather have an exit message that specifically tells 
> the user about their invalid configuration. Do you think that this is a bad 
> idea?

You'd fall through to this error message:
```
"'" << masterFlags.registry << "' is not a supported option for registry 
persistence";
```

I'd say this message is pretty much the same as the message you've added.  The 
`on Windows` part should hopefully be pretty obvious to the user :)


- Joseph


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


On Nov. 7, 2016, 10:23 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53313/
> ---
> 
> (Updated Nov. 7, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many agent tests currently use the master in some form. Of those,
> relatively few use the leveldb-based persistence. Since we cannot
> currently compile leveldb on Windows, and since running these tests on
> Windows involves compiling and running the master, this commit will
> remove the direct dependency on the replicated log for windows builds of
> the master.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 
>   src/master/main.cpp 2d2dfb7d632f3c7be1796efd8f0a1f4d18760261 
> 
> Diff: https://reviews.apache.org/r/53313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 53558: Added net::IP parsing template to the flags parsers.

[Armand Grillet]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Will be used to have flags of type `net::IP`.


Diffs
-

  3rdparty/stout/include/stout/flags/parse.hpp 
67a89c9068207b2197d60f3542962e82327a43a4 

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


Testing
---

make check (macOS)


Thanks,

Armand Grillet

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Qian Zhang]

> On Nov. 8, 2016, 8:38 a.m., Qian Zhang wrote:
> > I do not quite understand the intention of this patch. The description of 
> > this patch says that `Previously, the headers did not contain includes for 
> > any of the classes used in the file.`, but actually `stout/os.hpp` has been 
> > already included which almost has all the needed headers, like 
> > `stout/nothing.hpp`, `stout/try.hpp`, etc. So I guess the intention of this 
> > patch is to remove the inclusion of `stout/os.hpp` and instead include each 
> > needed headers directly, right? But I still see `stout/os.hpp` in both 
> > `utils.hpp` and `utils.cpp` in this patch which makes me a bit confused.
> 
> Joseph Wu wrote:
> The style for our include headers is to include all dependencies, so that 
> changing the include headers of a dependency does not break any upstream code.
> 
> i.e. 
> foo.hpp:
> ```
> #include 
> 
> typedef std::string Fooo;
> ```
> 
> bar.hpp:
> ```
> #include 
> #include  // <- Need this because we use it in this file.
> 
> #include "foo.hpp"
> 
> std::map bar;
> ```
> 
> The exception is any headers that are effectively aliases to other 
> headers, like `stout/lambda.hpp` (to various `` types) or 
> `src/messages/messages.hpp` (to the protobuf `messages.pb.h`).

Thanks for the explanation! So if a header is needed by `utils.cpp`, then 
`utils.cpp` should always include it even it has already been included in 
`utils.hpp`.

And I have a further comment: in your updated patch, I see in `utils.cpp`, you 
include `stout/os.hpp` and also `stout/try.hpp`, `stout/nothing.hpp`, etc. 
which are actually already included in `stout/os.hpp`. I think that is not what 
we are doing in Mesos, if we have included `stout/os.hpp` in a .cpp, we do not 
need to include the header files in it again. See the following code as 
examples:
https://github.com/apache/mesos/blob/1.0.1/src/slave/containerizer/mesos/provisioner/backend.cpp#L19
https://github.com/apache/mesos/blob/1.0.1/src/slave/containerizer/docker.cpp#L38

Both of the above two files use `Try` and `Nothing`, but they only include 
`stout/os.hpp` not `stout/try.hpp` and `stout/nothing.hpp`.


- Qian


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


On Nov. 8, 2016, 8:56 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53556/
> ---
> 
> (Updated Nov. 8, 2016, 8:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the headers in the provisioner utility.  Previously, the
> headers did not contain includes for any of the classes used in the
> file.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/utils.hpp 
> 4efce0ed67312417495af8c884ce26ef6affaccb 
>   src/slave/containerizer/mesos/provisioner/utils.cpp 
> 340cf48819679cdd8d08d76dcd2328234477a15f 
> 
> Diff: https://reviews.apache.org/r/53556/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49863: Added Test Modules that are loaded by mesos tests.

[Joseph Wu]

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




src/examples/CMakeLists.txt (line 21)


We don't really need to list the library name here.  Ditto on the other 
definitions.



src/examples/CMakeLists.txt (lines 114 - 122)


The `target_link_libraries` command below is smart enough to include 
dependencies from dependencies.  So if you link to `MESOS_TARGET`, you 
automagically get all the 3rdparty libraries too.

This means you don't need these lines.

See: https://cmake.org/cmake/help/v3.0/command/target_link_libraries.html

> Library dependencies are transitive by default with this signature. When 
this target is linked into another target then the libraries linked to this 
target will appear on the link line for the other target too.



src/examples/CMakeLists.txt (lines 126 - 138)


Even though we have the default linkage variable, it doesn't make sense to 
statically link modules.

These should be `SHARED`.  But on Windows, we should exclude the test 
modules.

---

Also, for readability, it would be nice to line up the variables 
vertically.  (Same below)



src/examples/cmake/ExamplesConfigure.cmake (line 47)


Everything in this file from here and below is un-used.


- Joseph Wu


On Oct. 4, 2016, 4:45 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49863/
> ---
> 
> (Updated Oct. 4, 2016, 4:45 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Builds shared libraries for dynamically loading them by various tests:
> libexamplemodule, libtestauthorizer, libtestisolator,
> libtestresource_estimator, libtestallocator, libtestcontainer_logger,
> libtestmastercontender, libtestanonymous, libtesthook,
> libtestmasterdetector, libtestauthentication, libtesthttpauthenticator,
> libtestqos_controller.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
>   cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
>   src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49863/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 53466: Added test to verify container status for nested containers.

[Jie Yu]

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

(Updated Nov. 8, 2016, 1:06 a.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


Bugs: MESOS-6465 and MESOS-6528
https://issues.apache.org/jira/browse/MESOS-6465
https://issues.apache.org/jira/browse/MESOS-6528


Repository: mesos


Description
---

This patch is used to verify that container status is properly set in
task status update for nested containers.


Diffs
-

  src/tests/default_executor_tests.cpp bb954fc48717b2c02abe1c98753b009ec3bb695a 
  src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 53561: Stopped taking task_id in scheduler::SendAcknowledge action in tests.

[Jie Yu]

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

Review request for mesos.


Repository: mesos


Description
---

Taking task_id is not necessary because the status update itself
(arg1) already has task_id in it.


Diffs
-

  src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 

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


Testing
---


Thanks,

Jie Yu

Re: Review Request 53466: Added test to verify container status for nested containers.

[Jie Yu]

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

(Updated Nov. 8, 2016, 1:05 a.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


Bugs: MESOS-6465 and MESOS-6528
https://issues.apache.org/jira/browse/MESOS-6465
https://issues.apache.org/jira/browse/MESOS-6528


Repository: mesos


Description
---

This patch is used to verify that container status is properly set in
task status update for nested containers.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp bb954fc48717b2c02abe1c98753b009ec3bb695a 
  src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 53467: Added ContainerStatus.container_id to JSON endpoints.

[Jie Yu]

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

(Updated Nov. 8, 2016, 1:05 a.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


Bugs: MESOS-6465 and MESOS-6528
https://issues.apache.org/jira/browse/MESOS-6465
https://issues.apache.org/jira/browse/MESOS-6528


Repository: mesos


Description
---

Added ContainerStatus.container_id to JSON endpoints.


Diffs (updated)
-

  src/common/http.cpp fb8454a97ad78c306d17b4222c14d68b3598175d 
  src/tests/slave_tests.cpp 8f1dbef081171421bf2761e24a4f267cbd015d0d 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Joseph Wu]

> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.cpp, line 22
> > 
> >
> > This can be removed here.
> > 
> > I think we don't double include in the impl when the corresponding 
> > header already includes a file.

See my response to Qian's question below.  TLDR: Yes we do :)


> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.hpp, line 26
> > 
> >
> > Not used here.
> 
> Qian Zhang wrote:
> It is required by `utils.cpp`.

Ah, right.  This should be moved into the `.cpp` file.


> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.hpp, line 23
> > 
> >
> > Not used here.

Removed.


> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.cpp, line 28
> > 
> >
> > Already included in header.

See above for these two.


> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.cpp, line 59
> > 
> >
> > This seems to require
> > 
> >  #include 
> >  #include 
> >  #include 

Good catch.


> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.cpp, line 61
> > 
> >
> > `stout/os/strerror.hpp`.

Included as an alias by `stout/error.hpp`.


> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.cpp, line 94
> > 
> >
> > `stout/posix/os.hpp`.

This is why we include `stout/os.hpp`.


> On Nov. 7, 2016, 3:07 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.cpp, line 104
> > 
> >
> > `stout/posix/rm.hpp`.

Included as an alias by `stout/os/rm.hpp`.


- Joseph


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


On Nov. 7, 2016, 4:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53556/
> ---
> 
> (Updated Nov. 7, 2016, 4:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the headers in the provisioner utility.  Previously, the
> headers did not contain includes for any of the classes used in the
> file.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/utils.hpp 
> 4efce0ed67312417495af8c884ce26ef6affaccb 
>   src/slave/containerizer/mesos/provisioner/utils.cpp 
> 340cf48819679cdd8d08d76dcd2328234477a15f 
> 
> Diff: https://reviews.apache.org/r/53556/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Joseph Wu]

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

(Updated Nov. 7, 2016, 4:56 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.


Changes
---

Addressed some parts of Benjamin's review.


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


Repository: mesos


Description
---

This fixes the headers in the provisioner utility.  Previously, the
headers did not contain includes for any of the classes used in the
file.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/utils.hpp 
4efce0ed67312417495af8c884ce26ef6affaccb 
  src/slave/containerizer/mesos/provisioner/utils.cpp 
340cf48819679cdd8d08d76dcd2328234477a15f 

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


Testing
---

make check (OSX)
make check (CentOS 7)
msbuild ... (Windows 10)


Thanks,

Joseph Wu

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Joseph Wu]

> On Nov. 7, 2016, 4:38 p.m., Qian Zhang wrote:
> > I do not quite understand the intention of this patch. The description of 
> > this patch says that `Previously, the headers did not contain includes for 
> > any of the classes used in the file.`, but actually `stout/os.hpp` has been 
> > already included which almost has all the needed headers, like 
> > `stout/nothing.hpp`, `stout/try.hpp`, etc. So I guess the intention of this 
> > patch is to remove the inclusion of `stout/os.hpp` and instead include each 
> > needed headers directly, right? But I still see `stout/os.hpp` in both 
> > `utils.hpp` and `utils.cpp` in this patch which makes me a bit confused.

The style for our include headers is to include all dependencies, so that 
changing the include headers of a dependency does not break any upstream code.

i.e. 
foo.hpp:
```
#include 

typedef std::string Fooo;
```

bar.hpp:
```
#include 
#include  // <- Need this because we use it in this file.

#include "foo.hpp"

std::map bar;
```

The exception is any headers that are effectively aliases to other headers, 
like `stout/lambda.hpp` (to various `` types) or 
`src/messages/messages.hpp` (to the protobuf `messages.pb.h`).


- Joseph


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


On Nov. 7, 2016, 2:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53556/
> ---
> 
> (Updated Nov. 7, 2016, 2:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the headers in the provisioner utility.  Previously, the
> headers did not contain includes for any of the classes used in the
> file.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/utils.hpp 
> 4efce0ed67312417495af8c884ce26ef6affaccb 
>   src/slave/containerizer/mesos/provisioner/utils.cpp 
> 340cf48819679cdd8d08d76dcd2328234477a15f 
> 
> Diff: https://reviews.apache.org/r/53556/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53555: Moved xattr utilities into stout/os/linux.hpp.

[Joseph Wu]

> On Nov. 7, 2016, 3:37 p.m., Benjamin Bannier wrote:
> > While it is clear how the current filesystem test can break under Windows, 
> > I do not understand why we need to wipe these utilities from all BSDs. The 
> > existing code already cleanly disabled the stout functions under Windows, 
> > and created useful wrappers for OS X which uses different parameters.
> > 
> > What you suggest here seems to remove potentially useful functionality, and 
> > also force users of this library to `ifdef` code instead of us solving this 
> > in the library. I would prefer us writing mostly `ifdef`-free user-code as 
> > it is easier to understand and maintain, and exposes code to more setups.
> > 
> > If you troubled by these functions living in the `stout/posix/` hierarchy, 
> > once could move all xattr related code to e.g., `stout/os/xattr.hpp`, and 
> > remove `stout/os/windows/xattr.hpp` and `stout/os/posix/xattr.hpp`. If 
> > there are issues under OS X we should fix them, and created related tests 
> > (the current test passes).

To address your three points:

When we add the various function prototypes in the `windows/*.hpp` files 
suffixed with `= delete;`, these are essentially TODOs.  Currently, there are 
only a few of these remaining (`chroot`, `su`, `getuid`, `getgid`, `chmod`, 
`chown`, `glob`, and `mknod`).  These are all functions defined in Posix, but 
with no functional equivalent in Windows.  While it doesn't hurt to add more 
deleted functions, it also doesn't do the codebase any good (because these will 
never be implemented). 

We can't avoid #ifdef's.  It's just a question of using `#ifdef __linux__` 
versus `#ifdef __WINDOWS__` (or similar).  We have **tons** of Linux-specific 
code and #ifdef-ing already.

`stout/os/xattr.hpp` wouldn't be appropriate, as there is obviously no 
cross-platform implementation.  We can always add back the OSX implementation 
of xattrs if we ever use it.  But currently, this code is only exercised in the 
Linux code.


> On Nov. 7, 2016, 3:37 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/tests/os/filesystem_tests.cpp, line 446
> > 
> >
> > This could just be `#ifndef __WINDOWS__`. It doesn't call into any 
> > Linux-specific functions to confirm the behavior of our wrapper, but 
> > instead uses observable effects that should work well under BSDs as well.

If you want to be complete, the #ifdef would be:
```
#ifdef __linux__ || __APPLE__ || __FreeBSD__ || __NetBSD__ || __OpenBSD__ || 
__DragonFly__
```

And we'd need to expand that for every potential OS that implements xattr :(
Simpler to just limit it to where we use it.


- Joseph


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


On Nov. 7, 2016, 2:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53555/
> ---
> 
> (Updated Nov. 7, 2016, 2:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `xattr` utilities are Linux specific syscalls.  Although these are
> defined in some form on some flavours of Posix (such as OSX and BSD),
> we do not intend to add the utility for all flavours of Posix.
> 
> Hence, this moves the `xattr` functions into `stout/os/linux.hpp`
> and removes any Posix-flavour-specific #ifdef-ing.
> 
> This also fixes the Windows build by properly #ifdef-ing the related
> filesystem test.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 
>   3rdparty/stout/include/stout/os.hpp 
> bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba 
>   3rdparty/stout/include/stout/os/linux.hpp 
> bf12e0b0577810c64cc8276ff0d987524327ffcd 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp 
> 518940fdffab38ad97cf229078c4494fa944e1d8 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp 
> 3157f72c5a0d5fdf59dccb3a34b154e0bb719bfa 
>   3rdparty/stout/include/stout/os/xattr.hpp 
> b86b50addc9bfeb5ddefa757ea74d357df46fbeb 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> 5c8f422443d0ea8e24b6a2bb506324966ce2e2ab 
> 
> Diff: https://reviews.apache.org/r/53555/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Qian Zhang]

> On Nov. 8, 2016, 7:07 a.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/provisioner/utils.hpp, line 26
> > 
> >
> > Not used here.

It is required by `utils.cpp`.


- Qian


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


On Nov. 8, 2016, 6:49 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53556/
> ---
> 
> (Updated Nov. 8, 2016, 6:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the headers in the provisioner utility.  Previously, the
> headers did not contain includes for any of the classes used in the
> file.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/utils.hpp 
> 4efce0ed67312417495af8c884ce26ef6affaccb 
>   src/slave/containerizer/mesos/provisioner/utils.cpp 
> 340cf48819679cdd8d08d76dcd2328234477a15f 
> 
> Diff: https://reviews.apache.org/r/53556/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Qian Zhang]

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



I do not quite understand the intention of this patch. The description of this 
patch says that `Previously, the headers did not contain includes for any of 
the classes used in the file.`, but actually `stout/os.hpp` has been already 
included which almost has all the needed headers, like `stout/nothing.hpp`, 
`stout/try.hpp`, etc. So I guess the intention of this patch is to remove the 
inclusion of `stout/os.hpp` and instead include each needed headers directly, 
right? But I still see `stout/os.hpp` in both `utils.hpp` and `utils.cpp` in 
this patch which makes me a bit confused.

- Qian Zhang


On Nov. 8, 2016, 6:49 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53556/
> ---
> 
> (Updated Nov. 8, 2016, 6:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the headers in the provisioner utility.  Previously, the
> headers did not contain includes for any of the classes used in the
> file.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/utils.hpp 
> 4efce0ed67312417495af8c884ce26ef6affaccb 
>   src/slave/containerizer/mesos/provisioner/utils.cpp 
> 340cf48819679cdd8d08d76dcd2328234477a15f 
> 
> Diff: https://reviews.apache.org/r/53556/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53554: Fixed header guards in stout/os/linux.hpp.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Nov. 8, 2016, 6:49 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53554/
> ---
> 
> (Updated Nov. 8, 2016, 6:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/linux.hpp 
> bf12e0b0577810c64cc8276ff0d987524327ffcd 
> 
> Diff: https://reviews.apache.org/r/53554/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53555: Moved xattr utilities into stout/os/linux.hpp.

[Benjamin Bannier]

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



While it is clear how the current filesystem test can break under Windows, I do 
not understand why we need to wipe these utilities from all BSDs. The existing 
code already cleanly disabled the stout functions under Windows, and created 
useful wrappers for OS X which uses different parameters.

What you suggest here seems to remove potentially useful functionality, and 
also force users of this library to `ifdef` code instead of us solving this in 
the library. I would prefer us writing mostly `ifdef`-free user-code as it is 
easier to understand and maintain, and exposes code to more setups.

If you troubled by these functions living in the `stout/posix/` hierarchy, once 
could move all xattr related code to e.g., `stout/os/xattr.hpp`, and remove 
`stout/os/windows/xattr.hpp` and `stout/os/posix/xattr.hpp`. If there are 
issues under OS X we should fix them, and created related tests (the current 
test passes).


3rdparty/stout/tests/os/filesystem_tests.cpp (line 445)


This could just be `#ifndef __WINDOWS__`. It doesn't call into any 
Linux-specific functions to confirm the behavior of our wrapper, but instead 
uses observable effects that should work well under BSDs as well.


- Benjamin Bannier


On Nov. 7, 2016, 11:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53555/
> ---
> 
> (Updated Nov. 7, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `xattr` utilities are Linux specific syscalls.  Although these are
> defined in some form on some flavours of Posix (such as OSX and BSD),
> we do not intend to add the utility for all flavours of Posix.
> 
> Hence, this moves the `xattr` functions into `stout/os/linux.hpp`
> and removes any Posix-flavour-specific #ifdef-ing.
> 
> This also fixes the Windows build by properly #ifdef-ing the related
> filesystem test.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 
>   3rdparty/stout/include/stout/os.hpp 
> bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba 
>   3rdparty/stout/include/stout/os/linux.hpp 
> bf12e0b0577810c64cc8276ff0d987524327ffcd 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp 
> 518940fdffab38ad97cf229078c4494fa944e1d8 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp 
> 3157f72c5a0d5fdf59dccb3a34b154e0bb719bfa 
>   3rdparty/stout/include/stout/os/xattr.hpp 
> b86b50addc9bfeb5ddefa757ea74d357df46fbeb 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> 5c8f422443d0ea8e24b6a2bb506324966ce2e2ab 
> 
> Diff: https://reviews.apache.org/r/53555/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53556: Fixed and updated provisioner utility headers.

[Benjamin Bannier]

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




src/slave/containerizer/mesos/provisioner/utils.hpp (line 23)


Not used here.



src/slave/containerizer/mesos/provisioner/utils.hpp (line 26)


Not used here.



src/slave/containerizer/mesos/provisioner/utils.cpp (line 22)


This can be removed here.

I think we don't double include in the impl when the corresponding header 
already includes a file.



src/slave/containerizer/mesos/provisioner/utils.cpp (line 25)


Already included in header.



src/slave/containerizer/mesos/provisioner/utils.cpp (line 28)


Already included in header.



src/slave/containerizer/mesos/provisioner/utils.cpp (line 59)


This seems to require

 #include 
 #include 
 #include 



src/slave/containerizer/mesos/provisioner/utils.cpp (line 61)


`stout/os/strerror.hpp`.



src/slave/containerizer/mesos/provisioner/utils.cpp (line 74)


`stout/path.hpp`.



src/slave/containerizer/mesos/provisioner/utils.cpp (line 94)


`stout/posix/os.hpp`.



src/slave/containerizer/mesos/provisioner/utils.cpp (line 104)


`stout/posix/rm.hpp`.


- Benjamin Bannier


On Nov. 7, 2016, 11:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53556/
> ---
> 
> (Updated Nov. 7, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the headers in the provisioner utility.  Previously, the
> headers did not contain includes for any of the classes used in the
> file.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/utils.hpp 
> 4efce0ed67312417495af8c884ce26ef6affaccb 
>   src/slave/containerizer/mesos/provisioner/utils.cpp 
> 340cf48819679cdd8d08d76dcd2328234477a15f 
> 
> Diff: https://reviews.apache.org/r/53556/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 52310: Switch the uid of the binary if a user is passed from the lib_logrotate.

[Sivaram Kannan]

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

(Updated Nov. 7, 2016, 10:56 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Switch the uid of the binary if a user is passed from the lib_logrotate.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52308: Add variable user to handle switchUser passed from executor.

[Sivaram Kannan]

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

(Updated Nov. 7, 2016, 10:56 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add variable user to handle switchUser passed from executor.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.hpp 
d1db69236f5a9b1dbb3113ad02218a512afdb46b 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 53473: Add new param user to logrotate's prepare function.

[Sivaram Kannan]

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

(Updated Nov. 7, 2016, 10:56 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add new param user to logrotate's prepare function.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
939974736f9eb744c83036e074718d2a1eba8b0a 
  src/slave/container_loggers/lib_logrotate.hpp 
28fdf3bdcc66d473521b377f66ab0b48f6900f58 
  src/slave/container_loggers/lib_logrotate.cpp 
53698d339f0f4d2dc916b53239ca0c36bbebcd42 
  src/slave/container_loggers/logrotate.hpp 
d1db69236f5a9b1dbb3113ad02218a512afdb46b 
  src/slave/container_loggers/sandbox.hpp 
e0aeb32a9ec83af049af8a10010b819c1d8b25d8 
  src/slave/container_loggers/sandbox.cpp 
cc263ebef7e0c3e778fabafa49faa6dd315adc45 
  src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/tests/container_logger_tests.cpp 1bb94a8461e481983f25a44737e4011ed5fc4b1f 

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


Testing
---

Run the mesos-logrotate-logger with un-priviledged user and verify whether the 
logs are getting rotated.
Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Review Request 53555: Moved xattr utilities into stout/os/linux.hpp.

[Joseph Wu]

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

Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.


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


Repository: mesos


Description
---

The `xattr` utilities are Linux specific syscalls.  Although these are
defined in some form on some flavours of Posix (such as OSX and BSD),
we do not intend to add the utility for all flavours of Posix.

Hence, this moves the `xattr` functions into `stout/os/linux.hpp`
and removes any Posix-flavour-specific #ifdef-ing.

This also fixes the Windows build by properly #ifdef-ing the related
filesystem test.


Diffs
-

  3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 
  3rdparty/stout/include/stout/os.hpp bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba 
  3rdparty/stout/include/stout/os/linux.hpp 
bf12e0b0577810c64cc8276ff0d987524327ffcd 
  3rdparty/stout/include/stout/os/posix/xattr.hpp 
518940fdffab38ad97cf229078c4494fa944e1d8 
  3rdparty/stout/include/stout/os/windows/xattr.hpp 
3157f72c5a0d5fdf59dccb3a34b154e0bb719bfa 
  3rdparty/stout/include/stout/os/xattr.hpp 
b86b50addc9bfeb5ddefa757ea74d357df46fbeb 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
5c8f422443d0ea8e24b6a2bb506324966ce2e2ab 

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


Testing
---

See next review.


Thanks,

Joseph Wu

Review Request 53556: Fixed and updated provisioner utility headers.

[Joseph Wu]

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

Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.


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


Repository: mesos


Description
---

This fixes the headers in the provisioner utility.  Previously, the
headers did not contain includes for any of the classes used in the
file.


Diffs
-

  src/slave/containerizer/mesos/provisioner/utils.hpp 
4efce0ed67312417495af8c884ce26ef6affaccb 
  src/slave/containerizer/mesos/provisioner/utils.cpp 
340cf48819679cdd8d08d76dcd2328234477a15f 

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


Testing
---

make check (OSX)
make check (CentOS 7)
msbuild ... (Windows 10)


Thanks,

Joseph Wu

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [53501]

Failed command: ./support/apply-review.sh -n -r 53501

Error:
2016-11-07 22:16:06 URL:https://reviews.apache.org/r/53501/diff/raw/ 
[2824/2824] -> "53501.patch" [1]
error: patch failed: src/tests/master_tests.cpp:91
error: src/tests/master_tests.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/15995/console

- Mesos ReviewBot


On Nov. 7, 2016, 4:46 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53501/
> ---
> 
> (Updated Nov. 7, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.
> 
> 
> Bugs: MESOS-4975
> https://issues.apache.org/jira/browse/MESOS-4975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to catch regressions to MESOS-4975.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3143af1 
> 
> Diff: https://reviews.apache.org/r/53501/diff/
> 
> 
> Testing
> ---
> 
> make check with this test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52587: Allow CREATE of shared volumes based on capability of framework.

[Jiang Yan Xu]

> On Nov. 4, 2016, 12:30 p.m., Neil Conway wrote:
> > src/master/validation.cpp, line 1532
> > 
> >
> > Can we use consistent tense here? The other volume error messages say 
> > "has been attempted" rather than "being attempted".
> > 
> > Also, the other error messages don't include the volume that failed 
> > validation, so we should probably be consistent.
> 
> Jiang Yan Xu wrote:
> +1 on the tense.
> 
> but re: "the other error messages don't include the volume that failed 
> validation" they don't include the volume but they include other things this 
> error message doesn't include. Whether the additional information is useful 
> depends on the failure IMO. Nevertheless I guess it's true we don't have to 
> say it's a shared volume separately because `stringify(volume)` reveals it.
> 
> 
> So overall the following look good?
> 
> ```
> ...
> 
> "Create volume operation for '" + stringify(volume) +
> "' has been attempted by framework '" +
> 
> ...
> ```
> 
> Neil Conway wrote:
> Why do you think this particular error message warrants including 
> `stringify(volume)` any more/less than the other places where a volume fails 
> validation? 
> 
> I'm not opposed to including more context in validation failure messages, 
> but I think we should also be consistent.

My critiera for adding `stringify(volume)` is that it will print the sharedness 
of the volume so it's obvious in the log w.r.t the operation: which volume is 
problematic, what makes it problematic, why it is problematic.

However examining the two other error messages closely, I think they should be 
given the same treatment. I'll send a RR.


- Jiang Yan


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


On Nov. 1, 2016, 9:54 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52587/
> ---
> 
> (Updated Nov. 1, 2016, 9:54 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6316
> https://issues.apache.org/jira/browse/MESOS-6316
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework issues a CREATE for a shared volume, we allow that
> operation only if the framework has opted in for the capability of
> SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we
> do not check as the operator API is not related to a specific
> framework.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 013bb592ba47b785c552e199633e4784e8aa71b1 
>   src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
>   src/master/validation.cpp f690a9eacd278b51a52f5588dbeea377df074435 
>   src/tests/master_validation_tests.cpp 
> a5d8610bd61822cdf55cbc8d7056e5cf8fecfa54 
>   src/tests/persistent_volume_tests.cpp 
> 6289009fe9ed0a57ba5eff46dbbe0633a75d2616 
> 
> Diff: https://reviews.apache.org/r/52587/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52645: Harden Mesos

[Aaron Wood]

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

(Updated Nov. 7, 2016, 9:53 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Specify the hash in the commit message for the macro we took 
(1a869696e4129279f7b99c3f9052717354b79a86).


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


Repository: mesos


Description
---

Use a default set of flags to provide additional security and hardening to 
Mesos. Additionally, check and catch more warnings/errors.


Diffs (updated)
-

  configure.ac 5380cbc 
  m4/ax_check_compile_flag.m4 PRE-CREATION 
  src/Makefile.am 5a47c93 

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


Testing
---

Compared the benchmarks with and without the flags being used. Also did a 
comparsion with the flags being used with and without optimizations and without 
the flags being used with and without optimizations. Overall the performance 
hit was very small with a 3-8% overhead (optimizations brings this down 
slightly). Most benchmarks were about 5% (or less) slower.


Thanks,

Aaron Wood

Re: Review Request 52696: Harden stout

[Aaron Wood]

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

(Updated Nov. 7, 2016, 9:52 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Specify the hash in the commit message for the macro we took 
(1a869696e4129279f7b99c3f9052717354b79a86).


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


Repository: mesos


Description
---

Use a default set of flags to provide additional security and hardening to 
stout. Additionally, check and catch more warnings/errors.


Diffs (updated)
-

  3rdparty/stout/Makefile.am 4e10ae2 
  3rdparty/stout/configure.ac f071f61 
  3rdparty/stout/m4/ax_check_compile_flag.m4 PRE-CREATION 

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


Testing
---

Compared the benchmarks with and without the flags being used. Also did a 
comparsion with the flags being used with and without optimizations and without 
the flags being used with and without optimizations. Overall the performance 
hit was very small with a 3-8% overhead (optimizations brings this down 
slightly). Most benchmarks were about 5% (or less) slower.


File Attachments


--enable-optimized with hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/18a2f590-75ad-49c5-a697-56b746f28cae__hardened-optimized.txt
Hardening applied but no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/a6e07766-80cc-4bd7-856d-8952cac12562__hardened-unoptimized.txt
--enable-optimized with no hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/046b37a9-5aff-4543-b3bb-5ac60daaf498__optimized.txt
No hardening applied and no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/3baa96cf-be05-4ac0-ad4c-ef571386e8f4__unoptimized.txt


Thanks,

Aaron Wood

Re: Review Request 52695: Harden libprocess

[Aaron Wood]

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

(Updated Nov. 7, 2016, 9:51 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Specify the hash in the commit message for the macro we took 
(1a869696e4129279f7b99c3f9052717354b79a86).


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


Repository: mesos


Description
---

Use a default set of flags to provide additional security and hardening to 
libprocess. Additionally, check and catch more warnings/errors.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 7131989 
  3rdparty/libprocess/configure.ac e65e5ca 
  3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 

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


Testing
---

Compared the benchmarks with and without the flags being used. Also did a 
comparsion with the flags being used with and without optimizations and without 
the flags being used with and without optimizations. Overall the performance 
hit was very small with a 3-8% overhead (optimizations brings this down 
slightly). Most benchmarks were about 5% (or less) slower.


File Attachments


--enable-optimized with hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/875c9e6e-c73b-4e3c-8265-0f7c6dc00351__hardened-optimized.txt
Hardening applied but no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/932d28a7-2d31-471a-b438-647841a6853c__hardened-unoptimized.txt
--enable-optimized with no hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/896944ea-9b31-4d62-b1b9-97fb4700a882__optimized.txt
No hardening applied and no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/b32667ce-3e3b-4d2b-b4f8-4c2404a0fc1c__unoptimized.txt


Thanks,

Aaron Wood

Re: Review Request 53467: Added ContainerStatus.container_id to JSON endpoints.

[Jie Yu]

> On Nov. 7, 2016, 7:41 p.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 1815
> > 
> >
> > why this change? is it backwards compatible?
> > 
> > This method is called by `getContainers()` which is part of v1 API and 
> > we try to avoid calling hand coded `model` functions from v1.

It is backward compatible becasue model basically does protobuf to json 
conversion, except for ContainerID, which we want to use `x.y.z` rather than a 
strcutured container ID json.


- Jie


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


On Nov. 4, 2016, 7:40 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53467/
> ---
> 
> (Updated Nov. 4, 2016, 7:40 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ContainerStatus.container_id to JSON endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp c2398177c9e5af12c7a20c02e92d3f3036a7a39a 
>   src/common/http.cpp fb8454a97ad78c306d17b4222c14d68b3598175d 
>   src/slave/http.cpp a32aca437cffd52bc2bcde859eedddca2038e3f1 
>   src/tests/slave_tests.cpp 8f1dbef081171421bf2761e24a4f267cbd015d0d 
> 
> Diff: https://reviews.apache.org/r/53467/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52695: Harden libprocess

[Benjamin Bannier]

> On Nov. 2, 2016, 10:32 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/m4/ax_check_compile_flag.m4, line 1
> > 
> >
> > For future updates it would be great if we'd write down the 
> > autoconf-archive release this file came from (it looks like the latest 
> > release containing it is `v2016.09.16`).
> 
> Aaron Wood wrote:
> I don't see any of the other macros having this information. Would you 
> just prefer a comment at the very top indicating the release?
> I took this from HEAD a few weeks back from this location 
> http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_check_compile_flag.m4
>  How can you tell it's from `v2016.09.16`?

Yes, currently this doesn't exist in many places, but I think it would be great 
to at least mention to upstream commit SHA1 you took this from, e.g., in this 
commit message. This can potentially make applying new upstream version of this 
file easier should we ever patch it.

What I did to tell what release this was is in is to checkout the upstream 
repo, and find which version it matched (`HEAD`). This can become harder if we 
patch the file substantially, or upstream reorganizes their code.


- Benjamin


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


On Nov. 7, 2016, 10:30 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52695/
> ---
> 
> (Updated Nov. 7, 2016, 10:30 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> libprocess. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 7131989 
>   3rdparty/libprocess/configure.ac e65e5ca 
>   3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52695/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> File Attachments
> 
> 
> --enable-optimized with hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/875c9e6e-c73b-4e3c-8265-0f7c6dc00351__hardened-optimized.txt
> Hardening applied but no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/932d28a7-2d31-471a-b438-647841a6853c__hardened-unoptimized.txt
> --enable-optimized with no hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/896944ea-9b31-4d62-b1b9-97fb4700a882__optimized.txt
> No hardening applied and no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/b32667ce-3e3b-4d2b-b4f8-4c2404a0fc1c__unoptimized.txt
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52695: Harden libprocess

[Aaron Wood]

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

(Updated Nov. 7, 2016, 9:30 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments, added a new flag to enable/disable hardening, apply 
hardening by default.


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


Repository: mesos


Description
---

Use a default set of flags to provide additional security and hardening to 
libprocess. Additionally, check and catch more warnings/errors.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 7131989 
  3rdparty/libprocess/configure.ac e65e5ca 
  3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 

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


Testing
---

Compared the benchmarks with and without the flags being used. Also did a 
comparsion with the flags being used with and without optimizations and without 
the flags being used with and without optimizations. Overall the performance 
hit was very small with a 3-8% overhead (optimizations brings this down 
slightly). Most benchmarks were about 5% (or less) slower.


File Attachments


--enable-optimized with hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/875c9e6e-c73b-4e3c-8265-0f7c6dc00351__hardened-optimized.txt
Hardening applied but no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/932d28a7-2d31-471a-b438-647841a6853c__hardened-unoptimized.txt
--enable-optimized with no hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/896944ea-9b31-4d62-b1b9-97fb4700a882__optimized.txt
No hardening applied and no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/b32667ce-3e3b-4d2b-b4f8-4c2404a0fc1c__unoptimized.txt


Thanks,

Aaron Wood

Re: Review Request 52696: Harden stout

[Aaron Wood]

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

(Updated Nov. 7, 2016, 9:27 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments, added a new flag to enable/disable hardening, apply 
hardening by default.


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


Repository: mesos


Description
---

Use a default set of flags to provide additional security and hardening to 
stout. Additionally, check and catch more warnings/errors.


Diffs (updated)
-

  3rdparty/stout/Makefile.am 4e10ae2 
  3rdparty/stout/configure.ac f071f61 
  3rdparty/stout/m4/ax_check_compile_flag.m4 PRE-CREATION 

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


Testing
---

Compared the benchmarks with and without the flags being used. Also did a 
comparsion with the flags being used with and without optimizations and without 
the flags being used with and without optimizations. Overall the performance 
hit was very small with a 3-8% overhead (optimizations brings this down 
slightly). Most benchmarks were about 5% (or less) slower.


File Attachments


--enable-optimized with hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/18a2f590-75ad-49c5-a697-56b746f28cae__hardened-optimized.txt
Hardening applied but no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/a6e07766-80cc-4bd7-856d-8952cac12562__hardened-unoptimized.txt
--enable-optimized with no hardening applied
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/046b37a9-5aff-4543-b3bb-5ac60daaf498__optimized.txt
No hardening applied and no --enable-optimized
  
https://reviews.apache.org/media/uploaded/files/2016/11/02/3baa96cf-be05-4ac0-ad4c-ef571386e8f4__unoptimized.txt


Thanks,

Aaron Wood

Re: Review Request 52645: Harden Mesos

[Aaron Wood]

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

(Updated Nov. 7, 2016, 9:25 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments, added a new flag to enable/disable hardening, apply 
hardening by default.


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


Repository: mesos


Description
---

Use a default set of flags to provide additional security and hardening to 
Mesos. Additionally, check and catch more warnings/errors.


Diffs (updated)
-

  configure.ac 5380cbc 
  m4/ax_check_compile_flag.m4 PRE-CREATION 
  src/Makefile.am 5a47c93 

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


Testing
---

Compared the benchmarks with and without the flags being used. Also did a 
comparsion with the flags being used with and without optimizations and without 
the flags being used with and without optimizations. Overall the performance 
hit was very small with a 3-8% overhead (optimizations brings this down 
slightly). Most benchmarks were about 5% (or less) slower.


Thanks,

Aaron Wood

Re: Review Request 52645: Harden Mesos

[Aaron Wood]

> On Nov. 2, 2016, 9:33 a.m., Benjamin Bannier wrote:
> > src/Makefile.am, line 120
> > 
> >
> > Not sure we want to remove the existing `-Werror`.
> 
> Aaron Wood wrote:
> From a discussion with a few people on Slack it sounded like this was in 
> the wrong place to begin with (along with `-Wall` and `-Wsign-compare`). I 
> found that when I had moved this to `AM_CXXFLAGS` it would actually apply 
> hard errors for warnings where it never did before. The issue now with 
> putting this back into `AM_CXXFLAGS` is that all of the `clang: error: 
> argument unused during compilation: '-pthread'` warnings you get when 
> building Mesos which were always there now fail the whole thing. I think we'd 
> have to unravel the overall build process more to fix that issue.

Dropping this for now. If you feel it's really important we can open it again :)


- Aaron


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


On Nov. 1, 2016, 7:37 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52645/
> ---
> 
> (Updated Nov. 1, 2016, 7:37 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> Mesos. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   configure.ac c8d48be 
>   m4/ax_check_compile_flag.m4 PRE-CREATION 
>   src/Makefile.am c2f9e44 
> 
> Diff: https://reviews.apache.org/r/52645/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 53539: Tweaked parameter name in stout.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53536, 53537, 53538, 53539]

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 Nov. 7, 2016, 4:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53539/
> ---
> 
> (Updated Nov. 7, 2016, 4:44 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure consistency between declaration and definition. Spotted via
> clang-tidy.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> 9734561549eea867d57f14a0ab71febeb9dddc06 
> 
> Diff: https://reviews.apache.org/r/53539/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Note that there are lots of other mismatches in names between declarations 
> and definitions, but most of the ones I didn't fix seem to be intentional: 
> e.g., declaring a parameter named `t` but using `t_` in the definition 
> (typically because the implementation assigns `t_` to a member field named 
> `t`).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

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

(Updated Nov. 7, 2016, 12:13 p.m.)


Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.


Changes
---

Addressed comments other than to pass any all configs through environment 
variables. I suggest we do it systematically based on the disussion conclusion 
instead of having it lumped together with this fix.


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


Repository: mesos


Description
---

Used an environment variable to pass command environment.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/containerizer/mesos/main.cpp 
1a0e765ddb6d8519426b8d47067efdfa3432e07a 

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


Testing
---

make check.

No new tests are written but the fact the existing tests that run exectuors 
depends on this to work correctly is a proof.


Thanks,

Jiang Yan Xu

Re: Review Request 53467: Added ContainerStatus.container_id to JSON endpoints.

[Vinod Kone]

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




src/slave/http.cpp (line 1815)


why this change? is it backwards compatible?

This method is called by `getContainers()` which is part of v1 API and we 
try to avoid calling hand coded `model` functions from v1.


- Vinod Kone


On Nov. 4, 2016, 7:40 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53467/
> ---
> 
> (Updated Nov. 4, 2016, 7:40 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ContainerStatus.container_id to JSON endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp c2398177c9e5af12c7a20c02e92d3f3036a7a39a 
>   src/common/http.cpp fb8454a97ad78c306d17b4222c14d68b3598175d 
>   src/slave/http.cpp a32aca437cffd52bc2bcde859eedddca2038e3f1 
>   src/tests/slave_tests.cpp 8f1dbef081171421bf2761e24a4f267cbd015d0d 
> 
> Diff: https://reviews.apache.org/r/53467/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53466: Added test to verify container status for nested containers.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 4, 2016, 7:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53466/
> ---
> 
> (Updated Nov. 4, 2016, 7:08 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is used to verify that container status is properly set in
> task status update for nested containers.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> db953d39a95e7e95871054114a0a9bbeded46224 
>   src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 
> 
> Diff: https://reviews.apache.org/r/53466/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53537: Avoided unnecessary copies in Mesos.

[Neil Conway]

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

(Updated Nov. 7, 2016, 7:32 p.m.)


Review request for mesos, Benjamin Bannier and Michael Park.


Changes
---

Squelched a few more copies.


Repository: mesos


Description
---

Spotted via clang-tidy.


Diffs (updated)
-

  src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 
  src/module/manager.cpp 18ee65796dd5744d36a645e7ca1f5ab0fbce98a5 
  src/slave/http.cpp 381d9c3ca0e6f61e8649603cbf770b8f01de4bf9 
  src/tests/containerizer.cpp c3bcb85d245a0e95b377059802cd89617eb5e25c 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
b5797574ca2bfe3a005c80a3ba6bb6965c54cc95 
  src/tests/health_check_tests.cpp e473c98da8dfeb0e05b9dec7d4be1ce227510cf5 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 53465: Fixed the container status for nested containers.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53465/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, the ContainerStatus inside the TaskStatus for a
> task in the default executor is not correct. The agent always gets the
> container status of the top level container, even if the task might
> correspond to a nested container.
> 
> This patch fixed this issue by letting the executor set the container
> ID for the task (because only it has the knowledge about this
> mapping), and the agent will use that container ID to get the proper
> container status.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp b8a88ad6e361720dc7d02c9423db3e67226f779f 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 2f6723c64261fb3295626d6479fe844fb23b0650 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 939142e36b926d9e4201d35dedd25e32e9f8c63c 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> Diff: https://reviews.apache.org/r/53465/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52696: Harden stout

[Aaron Wood]

> On Nov. 2, 2016, 9:33 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/Makefile.am, line 27
> > 
> >
> > I am not a big fan of unconditionally omitting frame pointers as this 
> > gives the optimizer one less register to work with. Unfortunately one 
> > cannot easily tell the actual impact of this from the info here. Is this 
> > strictly needed here or just nice to have?

Going to drop this since we've all agreed on Slack to have the frame pointer 
modification done in a separate patch.


- Aaron


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


On Nov. 2, 2016, 3:35 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52696/
> ---
> 
> (Updated Nov. 2, 2016, 3:35 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> stout. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4e10ae2 
>   3rdparty/stout/configure.ac cbb0fdb 
>   3rdparty/stout/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52696/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> File Attachments
> 
> 
> --enable-optimized with hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/18a2f590-75ad-49c5-a697-56b746f28cae__hardened-optimized.txt
> Hardening applied but no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/a6e07766-80cc-4bd7-856d-8952cac12562__hardened-unoptimized.txt
> --enable-optimized with no hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/046b37a9-5aff-4543-b3bb-5ac60daaf498__optimized.txt
> No hardening applied and no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/3baa96cf-be05-4ac0-ad4c-ef571386e8f4__unoptimized.txt
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52695: Harden libprocess

[Aaron Wood]

> On Nov. 2, 2016, 9:32 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/Makefile.am, line 30
> > 
> >
> > I am not a big fan of unconditionally omitting frame pointers as this 
> > gives the optimizer one less register to work with. Unfortunately one 
> > cannot easily tell the actual impact of this from the info here. Is this 
> > strictly needed here or just nice to have?
> 
> James Peach wrote:
> The performance benefit of omitting frame pointers is likely to be 
> marginal on x64_64, if it is a win at all. The rationale for adding this is 
> that it makes stack walking reliable in all cases, so debugability is 
> improved and you can get reasonable results when uting `perf`. Since most 
> users will build with default options I suggested to Aaron that we should 
> make it the default.
> 
> Benjamin Bannier wrote:
> Thanks James, that makes sense.
> 
> Since this seems all related to debugability what about enabling it _only 
> for builds with `--enable-debug`_ (e.g., perf results already now also don't 
> necessarily give full info w/o debug symbols)? Tangentially related, tcmalloc 
> can fail in debug builds with omitted frame pointers, so disabling 
> `omit-frame-pointer` in debug builds might safe us from some future 
> headaches, https://bugs.chromium.org/p/chromium/issues/detail?id=636489.
> 
> `stack-protector-strong` can significantly increase the binary size, and 
> we should either only enable it for e.g., debug builds, or give users a 
> `configure` knob to disable it.
> 
> For using `FORTIFY_SOURCE` I think we also need be a little more careful. 
> Support for it is somewhat broken in clang 
> (https://llvm.org/bugs/show_bug.cgi?id=16821), it only has useful effects in 
> builds with some level of optimization, and can e.g., mess up reports from 
> sanitizers injected by users. I can see good uses for a `configure` flag to 
> disable this compiler flag, but I am not sure what the default should be.

Going to drop this since we've all agreed on Slack to have the frame pointer 
modification done in a separate patch.


- Aaron


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


On Nov. 2, 2016, 3:14 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52695/
> ---
> 
> (Updated Nov. 2, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> libprocess. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 7131989 
>   3rdparty/libprocess/configure.ac 1644035 
>   3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52695/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> File Attachments
> 
> 
> --enable-optimized with hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/875c9e6e-c73b-4e3c-8265-0f7c6dc00351__hardened-optimized.txt
> Hardening applied but no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/932d28a7-2d31-471a-b438-647841a6853c__hardened-unoptimized.txt
> --enable-optimized with no hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/896944ea-9b31-4d62-b1b9-97fb4700a882__optimized.txt
> No hardening applied and no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/b32667ce-3e3b-4d2b-b4f8-4c2404a0fc1c__unoptimized.txt
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52645: Harden Mesos

[Aaron Wood]

> On Nov. 2, 2016, 9:33 a.m., Benjamin Bannier wrote:
> > src/Makefile.am, line 114
> > 
> >
> > I am not a big fan of unconditionally omitting frame pointers as this 
> > gives the optimizer one less register to work with. Unfortunately one 
> > cannot easily tell the actual impact of this from the info here. Is this 
> > strictly needed here or just nice to have?

Going to drop this since we've all agreed on Slack to have the frame pointer 
modification done in a separate patch.


- Aaron


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


On Nov. 1, 2016, 7:37 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52645/
> ---
> 
> (Updated Nov. 1, 2016, 7:37 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> Mesos. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   configure.ac c8d48be 
>   m4/ax_check_compile_flag.m4 PRE-CREATION 
>   src/Makefile.am c2f9e44 
> 
> Diff: https://reviews.apache.org/r/52645/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 53464: Added ContainerID to ContainerStatus.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53464/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ContainerID to ContainerStatus.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 905a34bb13c2fcd1b22ef4e9605405ef84711c25 
>   include/mesos/v1/mesos.proto b72069a6caff0b163d9d9cae45c3caa0b25de38f 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53464/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53463: Inlined '_status' method in MesosContainerizer as a lambda.

[Vinod Kone]

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


Ship it!




LGTM module kevin's comment.

- Vinod Kone


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53463/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Inlined '_status' method in MesosContainerizer as a lambda.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
> 
> Diff: https://reviews.apache.org/r/53463/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 53552: Remove unnecessary use of `typename` causing Windows build break.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

The use of `typename` in C++ is used to disambiguate between static
members and dependent names that use the same symbol. For example, if
you have some `T::iterator` and `T` happens to have a static member
called iterator, it is necessary to add `typename T::iterator` to
indicate that you want an iterator of `T` rather than to refer to teh
static member `T::iterator`.

In some cases we employ `typename` to make our intention clearer, even
though it is not strictly speaking necessary. While normally a good
habit, in the specific cases we change in this review, it causes MSVC to
explode.

This commit will remove these uses.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 53551: Fix namespace resolution issue for Windows builds.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

Fix namespace resolution issue for Windows builds.


Diffs
-

  src/tests/common/recordio_tests.cpp 3d14953ebb95ff26f1d91f7ddec2660c21ba005f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 53266: Fixed `DefaultExecutorTest.*` tests on hosts without Docker.

[Gastón Kleiman]

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

(Updated Nov. 7, 2016, 6:56 p.m.)


Review request for mesos, Till Toenshoff, Vinod Kone, and Jiang Yan Xu.


Changes
---

Addressed feedback.


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


Repository: mesos


Description (updated)
---

Some of the tests are parameterized by the containerizers.

Fixed the naming schema, so that the tests using the Docker
containerizer run only if Docker is available.

Now the tests with the Mesos containerizer also run if the tests are
started as a non-root user.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp db953d39a95e7e95871054114a0a9bbeded46224 

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


Testing
---

`sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*' 
--docker=/tmp/foo`
`sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`
`bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`


Thanks,

Gastón Kleiman

Re: Review Request 53550: Rename symbols in log.proto to avoid naming collision in win32 API.

[Alex Clemmer]

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



Worth noting is that the symbol `IGNORE_RESPONSE` is somewhat inconsistent with 
the naming convention of the other symbols here, _i.e._, we'd probably expect 
`ACCEPT` to be changed similarly to `ACCEPT_RESPONSE` instead. I felt that the 
benefit of changing all the symbols to be consistent was not high enough to 
change them all, though I'm open to other suggestions.

- Alex Clemmer


On Nov. 7, 2016, 6:49 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53550/
> ---
> 
> (Updated Nov. 7, 2016, 6:49 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The standard win32 headers define a macro, `IGNORE`, which presently
> collides with two uses of the same symbol in log.proto. This causes a
> compile error on Windows.
> 
> In this commit, we rename the symbol in the log.proto files. There are
> two primary reasons for this.
> 
> The first is because the trick we have previously applied to get around
> similar problems (in which we `#undef` the macro, and re-define as a
> global constant) is made somewaht more complex by the fact that the
> log.proto headers are generated by protocol buffers. To implement this
> effectively, we'd have to individually `#undef` at every site we include
> log.pb.h, which is not a huge deal given the number of #include sites,
> but doesn't protect us against future build breaks.
> 
> The second is that the approach of re-naming the symbol in log.proto is
> not very invasive: we need to change a handful of places where the
> system is used, and we never have to think of the issue again. And,
> because it is an internal API, we don't require a major version bump to
> implement the change.
> 
> 
> Diffs
> -
> 
>   src/log/consensus.cpp 94ddf245c07ccd38d4fe828bc47c98ecf540243f 
>   src/log/coordinator.cpp 72ef0366633b4e4137fd303fa49f9efb166c80c9 
>   src/log/replica.cpp d596e617d4b4eab91245e0a88a3b9479fc75b813 
>   src/messages/log.proto 6a2c26be2afe411e6927cddebcfd9634b2b1b884 
> 
> Diff: https://reviews.apache.org/r/53550/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 53550: Rename symbols in log.proto to avoid naming collision in win32 API.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

The standard win32 headers define a macro, `IGNORE`, which presently
collides with two uses of the same symbol in log.proto. This causes a
compile error on Windows.

In this commit, we rename the symbol in the log.proto files. There are
two primary reasons for this.

The first is because the trick we have previously applied to get around
similar problems (in which we `#undef` the macro, and re-define as a
global constant) is made somewaht more complex by the fact that the
log.proto headers are generated by protocol buffers. To implement this
effectively, we'd have to individually `#undef` at every site we include
log.pb.h, which is not a huge deal given the number of #include sites,
but doesn't protect us against future build breaks.

The second is that the approach of re-naming the symbol in log.proto is
not very invasive: we need to change a handful of places where the
system is used, and we never have to think of the issue again. And,
because it is an internal API, we don't require a major version bump to
implement the change.


Diffs
-

  src/log/consensus.cpp 94ddf245c07ccd38d4fe828bc47c98ecf540243f 
  src/log/coordinator.cpp 72ef0366633b4e4137fd303fa49f9efb166c80c9 
  src/log/replica.cpp d596e617d4b4eab91245e0a88a3b9479fc75b813 
  src/messages/log.proto 6a2c26be2afe411e6927cddebcfd9634b2b1b884 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 53266: Fixed `DefaultExecutorTest.*` tests on hosts without Docker.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp (line 76)


s/ROOT_DOCKER_DockerContainerizer/ROOT_DOCKER_DockerAndMesosContainerizers/


- Vinod Kone


On Nov. 4, 2016, 5:12 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53266/
> ---
> 
> (Updated Nov. 4, 2016, 5:12 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6455
> https://issues.apache.org/jira/browse/MESOS-6455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some of the tests are parameterized by the containerizers.
> 
> Fixed the naming schema, so that the tests using the Docker
> containerizer run only if Docker is available.
> 
> Now the tests with the Mesos containerizer also run if the tests
> are started as a non-root user.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> db953d39a95e7e95871054114a0a9bbeded46224 
> 
> Diff: https://reviews.apache.org/r/53266/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*' 
> --docker=/tmp/foo`
> `sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`
> `bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 53313: Windows: Disable persistent state for Windows master.

[Alex Clemmer]

> On Oct. 31, 2016, 9:20 p.m., Joseph Wu wrote:
> > src/local/local.cpp, lines 214-228
> > 
> >
> > If you move `#ifdef` up one, you won't need to add the extra `EXIT(..)`.

I personally would rather have an exit message that specifically tells the user 
about their invalid configuration. Do you think that this is a bad idea?


- Alex


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


On Oct. 31, 2016, 7:31 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53313/
> ---
> 
> (Updated Oct. 31, 2016, 7:31 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Many agent tests currently use the master in some form. Of those,
> relatively few use the leveldb-based persistence. Since we cannot
> currently compile leveldb on Windows, and since running these tests on
> Windows involves compiling and running the master, this commit will
> remove the direct dependency on the replicated log for windows builds of
> the master.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 
>   src/master/main.cpp 2d2dfb7d632f3c7be1796efd8f0a1f4d18760261 
> 
> Diff: https://reviews.apache.org/r/53313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 53313: Windows: Disable persistent state for Windows master.

[Alex Clemmer]

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

(Updated Nov. 7, 2016, 6:23 p.m.)


Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

Many agent tests currently use the master in some form. Of those,
relatively few use the leveldb-based persistence. Since we cannot
currently compile leveldb on Windows, and since running these tests on
Windows involves compiling and running the master, this commit will
remove the direct dependency on the replicated log for windows builds of
the master.


Diffs (updated)
-

  src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 
  src/master/main.cpp 2d2dfb7d632f3c7be1796efd8f0a1f4d18760261 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 52642: Improved the validation of RESERVE operations.

[Gastón Kleiman]

> On Nov. 3, 2016, 10:39 p.m., Benjamin Mahler wrote:
> > src/tests/master_validation_tests.cpp, line 343
> > 
> >
> > Shouldn't we be checking that a framework can't remove a reservation 
> > for a role that doesn't match the framework's role? This technically 
> > shouldn't happen because we wouldn't have offered the resources to the 
> > framework, but this unfortunately requires some "non-local reasoning" to 
> > figure out why we don't have to check it.
> 
> Gastón Kleiman wrote:
> I'm creating a new RR with my new test. I'll add a test for the unreserve 
> operation to that RR.
> 
> Gastón Kleiman wrote:
> Please take a look at https://reviews.apache.org/r/53528/.

And at https://reviews.apache.org/r/53534/


- Gastón


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


On Nov. 7, 2016, 11:02 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52642/
> ---
> 
> (Updated Nov. 7, 2016, 11:02 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-6142
> https://issues.apache.org/jira/browse/MESOS-6142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Don't allow a reservation if the framework role doesn't match the role
> of all the resources.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 2f275f6c78b92e13bd7d38043b581b5a3555ee40 
>   src/master/master.cpp 15ad5a647fc71d36ffd149893b3b9448a7d99323 
>   src/master/validation.hpp ed878ef64c6bc49cfa1738f97c7af047f06ffee7 
>   src/master/validation.cpp 658f0dd1085c83cd82f8d2b0e2c4ec0ba785bb92 
>   src/tests/master_validation_tests.cpp 
> 8697bb2dc05c54553066ace58553f0e40dfcb5f5 
> 
> Diff: https://reviews.apache.org/r/52642/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 53529: Improved the readability of the master validation tests.

[Gastón Kleiman]

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

(Updated Nov. 7, 2016, 6 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
Park.


Changes
---

Rearranged the chain.


Repository: mesos


Description
---

Made all the validation tests in the file follow the same pattern.


Diffs
-

  src/tests/master_validation_tests.cpp 
8697bb2dc05c54553066ace58553f0e40dfcb5f5 

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


Testing
---

`make check`.


Thanks,

Gastón Kleiman

Review Request 53549: Fixed `operator<<` parameter naming in Mesos.

[Neil Conway]

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

Review request for mesos, Benjamin Bannier and Michael Park.


Repository: mesos


Description
---

By convention, we use `stream` for the first parameter to ostream
`operator<<` implementations.


Diffs
-

  src/linux/routing/handle.hpp bafeec2509f76c61382549c8d47ff9c5c10ec4cb 
  src/linux/routing/handle.cpp 3d8d2f147a1cefb3341a52de104eac25220792a6 
  src/tests/common/recordio_tests.cpp 3d14953ebb95ff26f1d91f7ddec2660c21ba005f 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 53548: Fixed `operator<<` parameter naming in stout.

[Neil Conway]

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

Review request for mesos, Benjamin Bannier and Michael Park.


Repository: mesos


Description
---

By convention, we use `stream` for the first parameter to ostream
`operator<<` implementations.


Diffs
-

  3rdparty/stout/include/stout/proc.hpp 
dd0d623e789b428d21fb98300e124fcc9288001e 
  3rdparty/stout/include/stout/version.hpp 
5ae1c73d1b32d09e8a32e0a5df17ebfa2fe7c4bc 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 53547: Fixed `operator<<` parameter naming in libprocess.

[Neil Conway]

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

Review request for mesos, Benjamin Bannier and Michael Park.


Repository: mesos


Description
---

By convention, we use `stream` for the first parameter to ostream
`operator<<` implementations.


Diffs
-

  3rdparty/libprocess/include/process/time.hpp 
554b291bc03d272fc92568f44da72ca7fcfe8838 
  3rdparty/libprocess/src/time.cpp 624ebc3611901a5fb3552dbe57b55623c1e818c5 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 50947: Removed isolator flag 'gpu/nvidia' for docker containerizer using GPU.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50947]

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 Nov. 7, 2016, 1:51 p.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50947/
> ---
> 
> (Updated Nov. 7, 2016, 1:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5975
> https://issues.apache.org/jira/browse/MESOS-5975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos containerizer uses isolator 'gpu/nvidia' for GPU isolation while
> docker and external containerizers do not need it anymore. This removed
> the 'gpu/nvidia' isolator check for docker and external containerizers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
> 2e722691475c84afae14009014ea70cc0fdd0e65 
> 
> Diff: https://reviews.apache.org/r/50947/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 53546: Added stub classes for rest cgroups subsystems.

[haosdent huang]

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

(Updated Nov. 7, 2016, 5:35 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added stub classes for rest cgroups subsystems.


Diffs
-

  src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
  src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
2f6723c64261fb3295626d6479fe844fb23b0650 
  src/slave/containerizer/mesos/isolators/cgroups/constants.hpp 
8c73efdfcfd3cf48a12cf81fcd075166dc51d7e3 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
bf7dc9e7efb183879bfb525cc7ecfac4cc53dbef 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/pid.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/pid.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Review Request 53546: Added stub classes for rest cgroups subsystems.

[haosdent huang]

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

Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.


Repository: mesos


Description
---

Added stub classes for rest cgroups subsystems.


Diffs
-

  src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
  src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
2f6723c64261fb3295626d6479fe844fb23b0650 
  src/slave/containerizer/mesos/isolators/cgroups/constants.hpp 
8c73efdfcfd3cf48a12cf81fcd075166dc51d7e3 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
bf7dc9e7efb183879bfb525cc7ecfac4cc53dbef 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/pid.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/pid.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 53545: Fixed the 'UnregisteredFrameworksAfterTearDown' test.

[Jiang Yan Xu]

> On Nov. 7, 2016, 9:14 a.m., Neil Conway wrote:
> > lgtm. Given the `frameworkInfo` change, you're confident that this test 
> > actually repros MESOS-4975, right?

Affirmatively. Just ran it with the fix reverted and it failed.


- Jiang Yan


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


On Nov. 7, 2016, 8:50 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53545/
> ---
> 
> (Updated Nov. 7, 2016, 8:50 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the 'UnregisteredFrameworksAfterTearDown' test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp ed859ab82c9b625f70cd98fa320af87b8194b230 
> 
> Diff: https://reviews.apache.org/r/53545/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53545: Fixed the 'UnregisteredFrameworksAfterTearDown' test.

[Jiang Yan Xu]

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

(Updated Nov. 7, 2016, 9:24 a.m.)


Review request for mesos and Neil Conway.


Repository: mesos


Description
---

Fixed the 'UnregisteredFrameworksAfterTearDown' test.


Diffs
-

  src/tests/master_tests.cpp ed859ab82c9b625f70cd98fa320af87b8194b230 

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


Testing (updated)
---

make check.

Ran it with the fix 55322e7a206618e04109f462986c6a550afbd352 reverted and it 
failed.


Thanks,

Jiang Yan Xu

Re: Review Request 53545: Fixed the 'UnregisteredFrameworksAfterTearDown' test.

[Neil Conway]

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


Ship it!




lgtm. Given the `frameworkInfo` change, you're confident that this test 
actually repros MESOS-4975, right?

- Neil Conway


On Nov. 7, 2016, 4:50 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53545/
> ---
> 
> (Updated Nov. 7, 2016, 4:50 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the 'UnregisteredFrameworksAfterTearDown' test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp ed859ab82c9b625f70cd98fa320af87b8194b230 
> 
> Diff: https://reviews.apache.org/r/53545/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 53545: Fixed the 'UnregisteredFrameworksAfterTearDown' test.

[Jiang Yan Xu]

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

Review request for mesos and Neil Conway.


Repository: mesos


Description
---

Fixed the 'UnregisteredFrameworksAfterTearDown' test.


Diffs
-

  src/tests/master_tests.cpp ed859ab82c9b625f70cd98fa320af87b8194b230 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Jiang Yan Xu]

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

(Updated Nov. 7, 2016, 8:46 a.m.)


Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.


Changes
---

Neil's comments. The diff will be commit as a separate commit.


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


Repository: mesos


Description
---

Added a test to catch regressions to MESOS-4975.


Diffs (updated)
-

  src/tests/master_tests.cpp 3143af1 

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


Testing
---

make check with this test.


Thanks,

Jiang Yan Xu

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

[Aaron Wood]

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

(Updated Nov. 7, 2016, 4:45 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The hardening flags produced many new sign comparison errors in libprocess that 
need to be fixed for Mesos to compile/run.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 76dca0b 
  3rdparty/libprocess/src/encoder.hpp 005d1cc 
  3rdparty/libprocess/src/process.cpp ab2b5a9 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
  3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
  3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 

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


Testing
---

Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
Ran `make && make check && make bench`.


Thanks,

Aaron Wood

Re: Review Request 53539: Tweaked parameter name in stout.

[Neil Conway]

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

(Updated Nov. 7, 2016, 4:44 p.m.)


Review request for mesos, Benjamin Bannier and Michael Park.


Changes
---

Use `stream` instead of `out`.


Repository: mesos


Description
---

Ensure consistency between declaration and definition. Spotted via
clang-tidy.


Diffs (updated)
-

  3rdparty/stout/include/stout/json.hpp 
9734561549eea867d57f14a0ab71febeb9dddc06 

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


Testing
---

`make check`

Note that there are lots of other mismatches in names between declarations and 
definitions, but most of the ones I didn't fix seem to be intentional: e.g., 
declaring a parameter named `t` but using `t_` in the definition (typically 
because the implementation assigns `t_` to a member field named `t`).


Thanks,

Neil Conway

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Jiang Yan Xu]

> On Nov. 7, 2016, 8:28 a.m., Neil Conway wrote:
> >

Thanks for catching these! Will post a fix after I run make check.


- Jiang Yan


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


On Nov. 7, 2016, 8:18 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53501/
> ---
> 
> (Updated Nov. 7, 2016, 8:18 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.
> 
> 
> Bugs: MESOS-4975
> https://issues.apache.org/jira/browse/MESOS-4975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to catch regressions to MESOS-4975.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 
> 
> Diff: https://reviews.apache.org/r/53501/diff/
> 
> 
> Testing
> ---
> 
> make check with this test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Neil Conway]

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




src/tests/master_tests.cpp (line 3128)


Seems racy to me: what if the agent registers and receives the 
`SlaveRegisteredMessage` before the `FUTURE_MESSAGE` handler is registered?



src/tests/master_tests.cpp (line 3141)


Shouldn't this be using `frameworkInfo`?



src/tests/master_tests.cpp (line 3149)


"fully processes"



src/tests/master_tests.cpp (line 3174)


Style-wise, I'd opt for checking that `values.empty()` is true over 
checking for the size explicitly. (`size()` might not be constant-time for some 
containers, and in general it seems a bit clearer, semantically.)


- Neil Conway


On Nov. 7, 2016, 4:18 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53501/
> ---
> 
> (Updated Nov. 7, 2016, 4:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.
> 
> 
> Bugs: MESOS-4975
> https://issues.apache.org/jira/browse/MESOS-4975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to catch regressions to MESOS-4975.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 
> 
> Diff: https://reviews.apache.org/r/53501/diff/
> 
> 
> Testing
> ---
> 
> make check with this test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Jiang Yan Xu]

> On Nov. 5, 2016, 9:04 p.m., Anindya Sinha wrote:
> > src/tests/master_tests.cpp, line 3144
> > 
> >
> > nitpik: s/registered/frameworkRegistered, since we use 
> > `slaveRegistered` in this test?

The convention has been to use a varaible name that matches the method or 
message. In this case, `registered` is a method name and 
`slaveRegisteredMessage` is a message name. So I did 
s/slaveRegistered/slaveRegisteredMessage/ instead.


- Jiang Yan


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


On Nov. 4, 2016, 3:12 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53501/
> ---
> 
> (Updated Nov. 4, 2016, 3:12 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.
> 
> 
> Bugs: MESOS-4975
> https://issues.apache.org/jira/browse/MESOS-4975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to catch regressions to MESOS-4975.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 
> 
> Diff: https://reviews.apache.org/r/53501/diff/
> 
> 
> Testing
> ---
> 
> make check with this test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Jiang Yan Xu]

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

(Updated Nov. 7, 2016, 8:18 a.m.)


Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.


Changes
---

Review comments. NNFR.


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


Repository: mesos


Description
---

Added a test to catch regressions to MESOS-4975.


Diffs (updated)
-

  src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 

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


Testing
---

make check with this test.


Thanks,

Jiang Yan Xu

Re: Review Request 52886: Fix new sign comparison errors in stout produced by hardened flags

[Benjamin Bannier]

> On Nov. 2, 2016, 10:54 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/tests/json_tests.cpp, line 175
> > 
> >
> > Not yours, but the ordering of parameters (should be `expected, 
> > actual`) is wrong here and in many other places in this file.
> > 
> > It would be great if you could submit a separate cleanup patch for that.
> 
> Neil Conway wrote:
> FWIW, the next version of gtest changes this to treat the lhs and rhs of 
> an expectation equivalently 
> (https://github.com/google/googletest/commit/f364e188372e489230ef4e44e1aec6bcb08f3acf).
> 
> Aaron Wood wrote:
> We okay with dropping this then?

Sure.


- Benjamin


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


On Nov. 7, 2016, 5:11 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52886/
> ---
> 
> (Updated Nov. 7, 2016, 5:11 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in stout that 
> need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/cache_tests.cpp 0950c85 
>   3rdparty/stout/tests/flags_tests.cpp da4deb9 
>   3rdparty/stout/tests/hashmap_tests.cpp 2626d67 
>   3rdparty/stout/tests/hashset_tests.cpp 66e59db 
>   3rdparty/stout/tests/ip_tests.cpp b5a206f 
>   3rdparty/stout/tests/json_tests.cpp 2bc4c88 
>   3rdparty/stout/tests/linkedhashmap_tests.cpp 7a80769 
>   3rdparty/stout/tests/multimap_tests.cpp 488991b 
>   3rdparty/stout/tests/os/process_tests.cpp 4cb3b5f 
>   3rdparty/stout/tests/os/sendfile_tests.cpp e221689 
>   3rdparty/stout/tests/os/systems_tests.cpp 110ba5b 
>   3rdparty/stout/tests/os_tests.cpp 0b7ee07 
>   3rdparty/stout/tests/strings_tests.cpp 7dd3301 
> 
> Diff: https://reviews.apache.org/r/52886/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran make && make check && make bench.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 53540: Tweaked parameter names in Mesos.

[Neil Conway]

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

(Updated Nov. 7, 2016, 4:15 p.m.)


Review request for mesos, Benjamin Bannier and Michael Park.


Changes
---

Address review comments.


Repository: mesos


Description
---

Ensure consistency between declaration and definition. Spotted via
clang-tidy.


Diffs (updated)
-

  include/mesos/state/protobuf.hpp 47beb45ecb9d3486e9f8f8ce38d48264f512107d 
  src/common/http.hpp c2398177c9e5af12c7a20c02e92d3f3036a7a39a 
  src/common/protobuf_utils.cpp 7362b875ce1ffca6bc6376169a11494bdb9cf062 
  src/linux/capabilities.hpp 9c0bcf46c1bf8435eabb7410961ce52828ffbfea 
  src/linux/capabilities.cpp 3e30c4d8966246778d59245794885139e9d14dfe 
  src/log/coordinator.hpp b5f9c962a983af0d591e878d12b7fe8bb59cc867 
  src/master/master.hpp 87186c6e733a686f96528b1722fda1c287e9c881 
  src/master/master.cpp 15ad5a647fc71d36ffd149893b3b9448a7d99323 
  src/slave/slave.hpp c0a17657e684e87d555731d2e35ac15894c3c05b 
  src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
  src/tests/utils.cpp fc004a9c567898ffbc38a42cc2340dd57347a829 

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


Testing
---

`make check`

Note that there are lots of other mismatches in names between declarations and 
definitions, but most of the ones I didn't fix seem to be intentional: e.g., 
declaring a parameter named `t` but using `t_` in the definition (typically 
because the implementation assigns `t_` to a member field named `t`).


Thanks,

Neil Conway

Re: Review Request 52886: Fix new sign comparison errors in stout produced by hardened flags

[Aaron Wood]

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

(Updated Nov. 7, 2016, 4:11 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The hardening flags produced many new sign comparison errors in stout that need 
to be fixed for Mesos to compile/run.


Diffs (updated)
-

  3rdparty/stout/tests/cache_tests.cpp 0950c85 
  3rdparty/stout/tests/flags_tests.cpp da4deb9 
  3rdparty/stout/tests/hashmap_tests.cpp 2626d67 
  3rdparty/stout/tests/hashset_tests.cpp 66e59db 
  3rdparty/stout/tests/ip_tests.cpp b5a206f 
  3rdparty/stout/tests/json_tests.cpp 2bc4c88 
  3rdparty/stout/tests/linkedhashmap_tests.cpp 7a80769 
  3rdparty/stout/tests/multimap_tests.cpp 488991b 
  3rdparty/stout/tests/os/process_tests.cpp 4cb3b5f 
  3rdparty/stout/tests/os/sendfile_tests.cpp e221689 
  3rdparty/stout/tests/os/systems_tests.cpp 110ba5b 
  3rdparty/stout/tests/os_tests.cpp 0b7ee07 
  3rdparty/stout/tests/strings_tests.cpp 7dd3301 

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


Testing
---

Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
Ran make && make check && make bench.


Thanks,

Aaron Wood

Re: Review Request 53536: Avoided unnecessary copy in stout.

[Neil Conway]

> On Nov. 7, 2016, 3:56 p.m., Benjamin Bannier wrote:
> > As an aside, how did you use `support/mesos-tidy.sh` with custom `CHECKS` 
> > to find these, or just run plain `clang-tidy` on the files in the 
> > compilation database? I am asking since stout is a header-only library, and 
> > that with some `header-filter` there could be more issues in header files. 
> > Or else the code is just very clean ...
> 
> Benjamin Bannier wrote:
> One more thing. As you go through and fix lint categories, it would be 
> great if you could globally enable fixed clang-tidy checks in 
> https://github.com/apache/mesos/blob/bda1194e7b5b2c6f145d1f4b4bc22c5132bfca1d/support/mesos-tidy.sh#L29.

I just ran `docker run -e CHECKS=...` with various custom checks enabled.

Agreed on eventually adding more checks to the default list, but I was hoping 
to get us down to 0 `clang-tidy` warnings before I add more checks to the list 
:) There are still a few more warnings to address with the current set of 
checks. It would also be nice to update the docker image to use clang 3.9, 
since that apparently added a bunch of new checks over 3.8.


- Neil


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


On Nov. 7, 2016, 3:24 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53536/
> ---
> 
> (Updated Nov. 7, 2016, 3:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Spotted via clang-tidy.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/os_tests.cpp 0b7ee0723b6a608d6f110fa8ac16e0fd7b75ddea 
> 
> Diff: https://reviews.apache.org/r/53536/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52886: Fix new sign comparison errors in stout produced by hardened flags

[Aaron Wood]

> On Nov. 2, 2016, 9:54 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/tests/json_tests.cpp, line 175
> > 
> >
> > Not yours, but the ordering of parameters (should be `expected, 
> > actual`) is wrong here and in many other places in this file.
> > 
> > It would be great if you could submit a separate cleanup patch for that.
> 
> Neil Conway wrote:
> FWIW, the next version of gtest changes this to treat the lhs and rhs of 
> an expectation equivalently 
> (https://github.com/google/googletest/commit/f364e188372e489230ef4e44e1aec6bcb08f3acf).

We okay with dropping this then?


- Aaron


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


On Oct. 27, 2016, 7:32 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52886/
> ---
> 
> (Updated Oct. 27, 2016, 7:32 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in stout that 
> need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/cache_tests.cpp 0950c85 
>   3rdparty/stout/tests/flags_tests.cpp da4deb9 
>   3rdparty/stout/tests/hashmap_tests.cpp 2626d67 
>   3rdparty/stout/tests/hashset_tests.cpp 66e59db 
>   3rdparty/stout/tests/ip_tests.cpp b5a206f 
>   3rdparty/stout/tests/json_tests.cpp 2bc4c88 
>   3rdparty/stout/tests/linkedhashmap_tests.cpp 7a80769 
>   3rdparty/stout/tests/multimap_tests.cpp 488991b 
>   3rdparty/stout/tests/os/process_tests.cpp 4cb3b5f 
>   3rdparty/stout/tests/os/sendfile_tests.cpp e221689 
>   3rdparty/stout/tests/os/systems_tests.cpp 110ba5b 
>   3rdparty/stout/tests/os_tests.cpp 0b7ee07 
>   3rdparty/stout/tests/strings_tests.cpp 7dd3301 
> 
> Diff: https://reviews.apache.org/r/52886/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran make && make check && make bench.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 53536: Avoided unnecessary copy in stout.

[Benjamin Bannier]

> On Nov. 7, 2016, 4:56 p.m., Benjamin Bannier wrote:
> > As an aside, how did you use `support/mesos-tidy.sh` with custom `CHECKS` 
> > to find these, or just run plain `clang-tidy` on the files in the 
> > compilation database? I am asking since stout is a header-only library, and 
> > that with some `header-filter` there could be more issues in header files. 
> > Or else the code is just very clean ...

One more thing. As you go through and fix lint categories, it would be great if 
you could globally enable fixed clang-tidy checks in 
https://github.com/apache/mesos/blob/bda1194e7b5b2c6f145d1f4b4bc22c5132bfca1d/support/mesos-tidy.sh#L29.


- Benjamin


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


On Nov. 7, 2016, 4:24 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53536/
> ---
> 
> (Updated Nov. 7, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Spotted via clang-tidy.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/os_tests.cpp 0b7ee0723b6a608d6f110fa8ac16e0fd7b75ddea 
> 
> Diff: https://reviews.apache.org/r/53536/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52754: Remove unused code which now throws errors with the new hardening flags

[Aaron Wood]

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

(Updated Nov. 7, 2016, 3:51 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed recent comments.


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


Repository: mesos


Description
---

The hardening flags produced many new errors about unused functions, variables, 
etc. that need to be fixed for Mesos to compile/run.


Diffs (updated)
-

  3rdparty/libprocess/include/process/profiler.hpp f6fccfb 
  3rdparty/libprocess/src/profiler.cpp 0c4949e 
  3rdparty/libprocess/src/tests/benchmarks.cpp 945007c 
  3rdparty/libprocess/src/tests/process_tests.cpp a4af54a 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 

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


Testing
---

Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
Ran make && make check && make bench.


Thanks,

Aaron Wood