Re: Changing Mesos Minimum Compiler Version

[Joris Van Remoortere]

+1

On Thu, Apr 9, 2015 at 2:14 PM, Cody Maloney  wrote:

> As discussed in the last community meeting, we'd like to bump the minimum
> required compiler version from GCC 4.4 to GCC 4.8.
>
> The overall goals are to make Mesos development safer, faster, and reduce
> the maintenance burden. Currently a lot of stout has different codepaths
> for Pre-C++11 and Post-C++11compilers.
>
> Progress will be tracked in the JIRA: MESOS-2604
>
> The resulting supported compiler versions will be:
> GCC 4.8, GCC 4.9
> Clang 3.5, Clang 3.6
>
> For reference
> Compilers by Distribution Version: http://goo.gl/p1t1ls
>
> C++11 features supported by each compiler:
> https://gcc.gnu.org/projects/cxx0x.html
> http://clang.llvm.org/cxx_status.html
>

Changing Mesos Minimum Compiler Version

[Cody Maloney]

As discussed in the last community meeting, we'd like to bump the minimum
required compiler version from GCC 4.4 to GCC 4.8.

The overall goals are to make Mesos development safer, faster, and reduce
the maintenance burden. Currently a lot of stout has different codepaths
for Pre-C++11 and Post-C++11compilers.

Progress will be tracked in the JIRA: MESOS-2604

The resulting supported compiler versions will be:
GCC 4.8, GCC 4.9
Clang 3.5, Clang 3.6

For reference
Compilers by Distribution Version: http://goo.gl/p1t1ls

C++11 features supported by each compiler:
https://gcc.gnu.org/projects/cxx0x.html
http://clang.llvm.org/cxx_status.html

Re: Review Request 32967: Cleaned upstyle and comments in modules.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [32967]

All tests passed.

- Mesos ReviewBot


On April 9, 2015, 6:40 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32967/
> ---
> 
> (Updated April 9, 2015, 6:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Kapil Arya, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2565
> https://issues.apache.org/jira/browse/MESOS-2565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed minor style issues in modules and the misleading comments.
> 
> 
> Diffs
> -
> 
>   include/mesos/module.hpp 594212605127522a1b5f0d1335d6c208b499289b 
>   include/mesos/module/authenticatee.hpp 
> 94de988d524d6390a9f59edbc61f1f7cae514efd 
>   include/mesos/module/authenticator.hpp 
> 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 
>   include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce 
>   include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 
>   src/examples/test_anonymous_module.cpp 
> 859f4e19e0d50794172af4cff708edf171e6d02f 
>   src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 
>   src/examples/test_isolator_module.cpp 
> 2c303f5ff2f17d56a840a5e0108281873c17ce6a 
>   src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de 
> 
> Diff: https://reviews.apache.org/r/32967/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 32463: Improved testing around state.json endpoints.

[Ben Mahler]

> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, line 2816
> > 
> >
> > Why do you check the type here, but not above for e.g. `DATE`, `TIME`, 
> > and `USER`?

Ah nice catch, `"id"` needs it but the ones below don't need it!

For DATE, TIME, and USER, we can check equality (note that the `std::string 
build::DATE` implicitly becomes a `JSON::String`, which is `==` comparable to a 
`JSON::Value`). However, for `"id"`, we want to confirm that it's a non-empty 
`JSON::String`. If you check for `"" != value` then this holds for all 
non-empty strings, but it also holds for JSON::Number, JSON::Object, etc. So I 
check the type explicitly, make sense?


> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, line 66
> > 
> >
> > I noticed you do drive-by clean-up of some `process::` occurrences. Do 
> > you want to go further and remove all of them or prefer creating a newbie 
> > JIRA for that? There are e.g. `process::Message` and `Process::Time`.

Thanks for noting this, I originally added this to avoid having to add using 
clauses for a lot of things from process::http. However, we generally are 
avoiding using namespaces like this so I've reverted back to fine-grained using 
clauses, and a namespace alias at the top to deal with the `proces::http` 
problem.


> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, lines 2847-2849
> > 
> >
> > How about a one-liner here and below:
> > ```
> > EXPECT_TRUE(state.values["slaves"].as().values.empty());
> > ```

Thanks!


> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_tests.cpp, line 792
> > 
> >
> > Not yours, but can you do a drive-by fix and remove `process::`?

I'd like to avoid the noise in the diff since its in an unrelated test :)


> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, line 2814
> > 
> >
> > This fails on my Mac.
> > 
> > Also, since you're checking start time, you can add one more 
> > `EXPECT_GT` check caching time before starting master.

It turns out this fails because `Time::create` compensates for the advanced 
state the clock by adding `clock::advanced`. That has already been added from 
`Clock::now` in the endpoint and so the times are not equal.


> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_tests.cpp, lines 2807-2809
> > 
> >
> > Does it make sense to use `Clock::now().secs()` and compare doubles to 
> > avoid multiple conversions and mimic the way it is actually done in master 
> > code?

Hm.. but you have to pause the clock, yes?

I've gone with your suggestion since its cleaner, paused the Clock at the top 
of the test to ensure Clock::now matches the master's time.


> On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_tests.cpp, line 937
> > 
> >
> > Kill `process::` prefix?

Will leave the unrelated tests for now. :)


- Ben


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


On March 24, 2015, 11:42 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32463/
> ---
> 
> (Updated March 24, 2015, 11:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This improves the slave test, and adds a new test for the master, to help 
> prevent API regressions.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7 
>   src/tests/slave_tests.cpp fd09d65bf34136c0959419b451e54105300740c4 
> 
> Diff: https://reviews.apache.org/r/32463/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32558: Improve compile time of mesos by splitting flags

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [32558]

All tests passed.

- Mesos ReviewBot


On April 9, 2015, 6:27 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32558/
> ---
> 
> (Updated April 9, 2015, 6:27 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-292
> https://issues.apache.org/jira/browse/MESOS-292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Split the mesos master, slave flags into header + source file to 
> improve compile time significantly. Should be no functional changes.
> 
> Largely copy-paste, with a little reworking to remove unnecessary 
> headers from the .hpp, and order headers a little more reliably 
> (as well as remove duplicate includes) in the .cpp.
> 
> # Impact of these changes
> `time make check -j8`
> Before:
> make check  2732.93s user 103.89s system 514% cpu 9:11.63 total
> 
> After:
> make check  2421.18s user 96.60s system 506% cpu 8:16.67 total
> 
> 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, 
> Intel i7-4790K.
> 
> The numbers aren't incredibly stable (Other software running, 
> overclocking enabled, etc). They are a good general measure though of
> speedup.
> 
> I do a `make check` rather than just `make` because that is what devs
> do in a day-to-day flow, and if runtime is significantly impacted, it
> will show up.
> 
> Test steps:
> ```
> # Warm cache
> ../configure --disable-python --disable-java
> make check
> # Timed build
> rm -rf *
> ../configure --disable-python --disable-java
> time make check
> ```
> 
> Note: Similar, likely greater improvements in compile time would happen
> if stout were made to not be header only. Specifically .
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
>   src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 
>   src/logging/flags.cpp PRE-CREATION 
>   src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b 
>   src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd 
>   src/master/flags.cpp PRE-CREATION 
>   src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef 
>   src/slave/flags.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32558/diff/
> 
> 
> Testing
> ---
> 
> make check on ArchLinux with GCC 4.9.2
> make distcheck CentOS 7
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 32967: Cleaned upstyle and comments in modules.

[Aditi Dixit]

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

(Updated April 9, 2015, 6:40 p.m.)


Review request for mesos, Alexander Rukletsov, Kapil Arya, and Till Toenshoff.


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


Repository: mesos


Description
---

Fixed minor style issues in modules and the misleading comments.


Diffs (updated)
-

  include/mesos/module.hpp 594212605127522a1b5f0d1335d6c208b499289b 
  include/mesos/module/authenticatee.hpp 
94de988d524d6390a9f59edbc61f1f7cae514efd 
  include/mesos/module/authenticator.hpp 
6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 
  include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce 
  include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 
  src/examples/test_anonymous_module.cpp 
859f4e19e0d50794172af4cff708edf171e6d02f 
  src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 
  src/examples/test_isolator_module.cpp 
2c303f5ff2f17d56a840a5e0108281873c17ce6a 
  src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de 

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


Testing
---

make check


Thanks,

Aditi Dixit

Re: Review Request 32558: Improve compile time of mesos by splitting flags

[Cody Maloney]

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

(Updated April 9, 2015, 6:27 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Update includes per Ben Mahler's comments.


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


Repository: mesos


Description
---

Split the mesos master, slave flags into header + source file to 
improve compile time significantly. Should be no functional changes.

Largely copy-paste, with a little reworking to remove unnecessary 
headers from the .hpp, and order headers a little more reliably 
(as well as remove duplicate includes) in the .cpp.

# Impact of these changes
`time make check -j8`
Before:
make check  2732.93s user 103.89s system 514% cpu 9:11.63 total

After:
make check  2421.18s user 96.60s system 506% cpu 8:16.67 total

4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, 
Intel i7-4790K.

The numbers aren't incredibly stable (Other software running, 
overclocking enabled, etc). They are a good general measure though of
speedup.

I do a `make check` rather than just `make` because that is what devs
do in a day-to-day flow, and if runtime is significantly impacted, it
will show up.

Test steps:
```
# Warm cache
../configure --disable-python --disable-java
make check
# Timed build
rm -rf *
../configure --disable-python --disable-java
time make check
```

Note: Similar, likely greater improvements in compile time would happen
if stout were made to not be header only. Specifically .


Diffs (updated)
-

  src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
  src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 
  src/logging/flags.cpp PRE-CREATION 
  src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b 
  src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd 
  src/master/flags.cpp PRE-CREATION 
  src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef 
  src/slave/flags.cpp PRE-CREATION 

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


Testing
---

make check on ArchLinux with GCC 4.9.2
make distcheck CentOS 7


Thanks,

Cody Maloney

Re: Review Request 32558: Improve compile time of mesos by splitting flags

[Cody Maloney]

> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > Looks good Cody, just curious about many of the includes in the .cpp flag 
> > files, had a hard time telling how these were relevant.

Mainly just things left over from copying from the old. Removed most. Precise 
comments below.


> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > src/master/flags.hpp, lines 405-406
> > 
> >
> > Where is the mesos.hpp include for these? Looks like you removed it?

It was included implicitly by . Added it back.


> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > src/master/flags.cpp, lines 19-21
> > 
> >
> > I can't tell how these are relevant to this file, is there something 
> > I'm missing?

: Implicitly included through 'logging/flags.hpp'. That 
include may go away in time. This is included explicitly since we use add() 
which does a lot of template  intantiation which depends directly on 


Dropped json, protobuf. What they were once used for is covered by 
"common/parse.hpp".


> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > src/master/flags.cpp, lines 23-24
> > 
> >
> > How are these being used in this file?

Removed. Included via header now.


> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > src/master/flags.cpp, line 30
> > 
> >
> > Ditto here, where is this relevant?

Dropped. Not needed in this file, had copied the old includes over and reworked 
them to follow proper include order.


> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > src/slave/flags.cpp, line 19
> > 
> >
> > Why do you need this?

Same as why it is in "master/flags.hpp".


> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > src/slave/flags.cpp, line 21
> > 
> >
> > Why do you need this?

Fixed.


> On April 2, 2015, 7:45 p.m., Ben Mahler wrote:
> > src/slave/flags.cpp, line 25
> > 
> >
> > What's this one needed for?

Fixed.


- Cody


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


On April 9, 2015, 6:27 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32558/
> ---
> 
> (Updated April 9, 2015, 6:27 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-292
> https://issues.apache.org/jira/browse/MESOS-292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Split the mesos master, slave flags into header + source file to 
> improve compile time significantly. Should be no functional changes.
> 
> Largely copy-paste, with a little reworking to remove unnecessary 
> headers from the .hpp, and order headers a little more reliably 
> (as well as remove duplicate includes) in the .cpp.
> 
> # Impact of these changes
> `time make check -j8`
> Before:
> make check  2732.93s user 103.89s system 514% cpu 9:11.63 total
> 
> After:
> make check  2421.18s user 96.60s system 506% cpu 8:16.67 total
> 
> 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, 
> Intel i7-4790K.
> 
> The numbers aren't incredibly stable (Other software running, 
> overclocking enabled, etc). They are a good general measure though of
> speedup.
> 
> I do a `make check` rather than just `make` because that is what devs
> do in a day-to-day flow, and if runtime is significantly impacted, it
> will show up.
> 
> Test steps:
> ```
> # Warm cache
> ../configure --disable-python --disable-java
> make check
> # Timed build
> rm -rf *
> ../configure --disable-python --disable-java
> time make check
> ```
> 
> Note: Similar, likely greater improvements in compile time would happen
> if stout were made to not be header only. Specifically .
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
>   src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 
>   src/logging/flags.cpp PRE-CREATION 
>   src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b 
>   src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd 
>   src/master/flags.cpp PRE-CREATION 
>   src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef 
>   src/slave/flags.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32558/diff/
> 
> 
> Testing
> ---
> 
> make check on 

Re: Review Request 32939: Bumped the log level for dropped messages.

[Cody Maloney]

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

Ship it!


Ship It!

- Cody Maloney


On April 7, 2015, 9:48 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32939/
> ---
> 
> (Updated April 7, 2015, 9:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently these log messages are fired whenever we have delayed messages to a 
> process that is exited (e.g., log appends). I've also seen this a bunch in 
> our tests. Bumping this to VLOG(2) to keep fidelity of VLOG(1) high.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> cf4e36489be2c6aa01e838c1c71383f248deab5b 
> 
> Diff: https://reviews.apache.org/r/32939/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 32939: Bumped the log level for dropped messages.

[Cody Maloney]

> On April 8, 2015, 1:13 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2048
> > 
> >
> > While you're here, how about:
> > 
> > ```
> > VLOG(2) << "Dropping event for process: " << to;
> > ```
> > 
> > If you want to get a bit fancier, we could pull up JSONVisitor and use 
> > it to print what we're dropping, but I doubt that be of much value right 
> > now.
> 
> Ben Mahler wrote:
> s/:// since ':' usually precedes a reason for the issue.

I'd be wary of sharing the message data, since that could potentially contain 
secrets which shouldn't be shared. ProcessIDs on the other hand are always safe 
to print / already always available.


- Cody


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


On April 7, 2015, 9:48 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32939/
> ---
> 
> (Updated April 7, 2015, 9:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently these log messages are fired whenever we have delayed messages to a 
> process that is exited (e.g., log appends). I've also seen this a bunch in 
> our tests. Bumping this to VLOG(2) to keep fidelity of VLOG(1) high.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> cf4e36489be2c6aa01e838c1c71383f248deab5b 
> 
> Diff: https://reviews.apache.org/r/32939/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 30952: Adding scheduler validations to master

[Alexander Rukletsov]

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



src/master/master.cpp


Minor nit: I believe we use trailing underscores in this case. Think prime 
symbols.


- Alexander Rukletsov


On April 8, 2015, 9:11 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30952/
> ---
> 
> (Updated April 8, 2015, 9:11 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2290
> https://issues.apache.org/jira/browse/MESOS-2290
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> There is a scheduler validation 
> (https://github.com/apache/mesos/blob/master/src/scheduler/scheduler.cpp#L275)
>  missing in master. See motivation on MESOS-2288.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp dccd7c6 
>   src/master/validation.cpp 2f2e4ea 
>   src/sched/sched.cpp 66fd2b3 
>   src/scheduler/scheduler.cpp 584b042 
> 
> Diff: https://reviews.apache.org/r/30952/diff/
> 
> 
> Testing
> ---
> 
> make check and distcheck. Tests will be added with new HTTP API endpoints 
> tests.
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 32997: Removed stale 'Code Internals' document.

[Alexander Rukletsov]

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

Ship it!


Ship It!

- Alexander Rukletsov


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32997/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This document was fairly useless. In the future we should probably document 
> our overall patterns (e.g. stout / libprocess).
> 
> 
> Diffs
> -
> 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
>   docs/mesos-code-internals.md 7e5b8972807408670f36c5664f5b95d52c1a9a51 
> 
> Diff: https://reviews.apache.org/r/32997/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32975: MESOS-1790 Adds chown option to CommandInfo.URI

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [32975]

All tests passed.

- Mesos ReviewBot


On April 9, 2015, 4:40 p.m., Jim Klucar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32975/
> ---
> 
> (Updated April 9, 2015, 4:40 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-1790
> https://issues.apache.org/jira/browse/MESOS-1790
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added chown to CommandInfo.URI protocol buffer as an optional
> boolean that defaults to true, the current chown behavior.
> 
> The fetcher was updated to skip the os::chown operation if the chown
> boolean is set to false.
> 
> No documentation was updated.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 
>   src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
> 
> Diff: https://reviews.apache.org/r/32975/diff/
> 
> 
> Testing
> ---
> 
> Unit testing this functionality is difficult because it would require that 
> the user running the test to have permission to chown a file to someone other 
> than themselves. I didn't want to add that as a requirement to build. I added 
> the new field to the existing test cases just to see that they populate.
> 
> 
> Thanks,
> 
> Jim Klucar
> 
>

Re: Review Request 32975: MESOS-1790 Adds chown option to CommandInfo.URI

[Vinod Kone]

> On April 8, 2015, 9:57 p.m., Vinod Kone wrote:
> > src/tests/fetcher_tests.cpp, lines 112-116
> > 
> >
> > Add a note/todo here mentioning why you are setting these fields but 
> > not doing any asserts/expects on them.
> 
> Jim Klucar wrote:
> With the type_utils.cpp change, the EXPECT_EQ on lines 140/141 should be 
> checking the chown field. Should I still add text regarding how we can't 
> check this capability without assuming the user running the test has 
> permission to chown to some other user?

Yes. Because the EXPECT on #140 just checks that the chown field is properly 
propagated to fetcher environment but not that fetcher actually skips chown if 
chown is false.

If you write a FetcherTest (see examples later in the file) that actually runs 
a fetcher process, would you able to test the semantics? I'm thinking that you 
can create a non-existent user (random string) and set chown to false. Once 
fetcher process finishes, you can check that the owner of the fetched file 
hasn't changed to the user. Does that make sense?


> On April 8, 2015, 9:57 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 208
> > 
> >
> > please update type_utils.cpp to account for this field when comparing 
> > CommandInfos.
> 
> Jim Klucar wrote:
> done

as part of this change to type utils, please make sure that it is backwards 
compatible (e.g., scheduler using old ExecutorInfo protobuf with no chown field 
and master using the new ExecutorInfo protobuf with chown field). afaict, it is 
ok, but it is worth for you to go through that thought exercise.


- Vinod


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


On April 9, 2015, 4:40 p.m., Jim Klucar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32975/
> ---
> 
> (Updated April 9, 2015, 4:40 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-1790
> https://issues.apache.org/jira/browse/MESOS-1790
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added chown to CommandInfo.URI protocol buffer as an optional
> boolean that defaults to true, the current chown behavior.
> 
> The fetcher was updated to skip the os::chown operation if the chown
> boolean is set to false.
> 
> No documentation was updated.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 
>   src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
> 
> Diff: https://reviews.apache.org/r/32975/diff/
> 
> 
> Testing
> ---
> 
> Unit testing this functionality is difficult because it would require that 
> the user running the test to have permission to chown a file to someone other 
> than themselves. I didn't want to add that as a requirement to build. I added 
> the new field to the existing test cases just to see that they populate.
> 
> 
> Thanks,
> 
> Jim Klucar
> 
>

Re: Review Request 32975: MESOS-1790 Adds chown option to CommandInfo.URI

[Jim Klucar]

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

(Updated April 9, 2015, 4:40 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Added chown to CommandInfo.URI protocol buffer as an optional
boolean that defaults to true, the current chown behavior.

The fetcher was updated to skip the os::chown operation if the chown
boolean is set to false.

No documentation was updated.


Diffs (updated)
-

  include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 
  src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 

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


Testing
---

Unit testing this functionality is difficult because it would require that the 
user running the test to have permission to chown a file to someone other than 
themselves. I didn't want to add that as a requirement to build. I added the 
new field to the existing test cases just to see that they populate.


Thanks,

Jim Klucar

Re: Review Request 32999: Added a document for engineering principles and practices.

[Till Toenshoff]

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

Ship it!


This really is great Ben.


docs/home.md


Should the link have a trailing slash as the others do?


- Till Toenshoff


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32999/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a document for engineering principles and practices.
> 
> 
> Diffs
> -
> 
>   docs/engineering-principles-and-practices.md PRE-CREATION 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
> 
> Diff: https://reviews.apache.org/r/32999/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32995: Split up documentation for reporting bugs and submitting patches.

[Till Toenshoff]

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



docs/reporting-a-bug.md


"...please review the Assign the JIRA issue to yourself before you start 
working on it."

Not sure if this sentence makes any sense as is.


- Till Toenshoff


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32995/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> "Developers guide" was too generic, this splits up the documents to focus on 
> "what" the reader is interesting in doing.
> 
> 
> Diffs
> -
> 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
>   docs/mesos-developers-guide.md 6023f2cc4d37ecabc24699bef6bda32068e5b43d 
>   docs/reporting-a-bug.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32995/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32967: Cleaned upstyle and comments in modules.

[Alexander Rukletsov]

> On April 8, 2015, 1:20 p.m., Alexander Rukletsov wrote:
> > Please check my comment to https://reviews.apache.org/r/32608/. Also, 
> > please add Kapil Arya , myself  and a committer to the 
> > list of reviewers.
> 
> Aditi Dixit wrote:
> Would adding any committer be ok? Or should I email and ask a committer?
> 
> Alexander Rukletsov wrote:
> tillt will shepherd this.

As a consequence, let's remove Vinod and Ben Mahler from the list of reviewers. 
One committer (Till) is enough for this patch.


- Alexander


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


On April 8, 2015, 8:05 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32967/
> ---
> 
> (Updated April 8, 2015, 8:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Kapil Arya, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2565
> https://issues.apache.org/jira/browse/MESOS-2565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed minor style issues in modules and the misleading comments.
> 
> 
> Diffs
> -
> 
>   include/mesos/module/authenticatee.hpp 
> 94de988d524d6390a9f59edbc61f1f7cae514efd 
>   include/mesos/module/authenticator.hpp 
> 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 
>   include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce 
>   include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 
>   src/examples/test_anonymous_module.cpp 
> 859f4e19e0d50794172af4cff708edf171e6d02f 
>   src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 
>   src/examples/test_isolator_module.cpp 
> 2c303f5ff2f17d56a840a5e0108281873c17ce6a 
>   src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de 
> 
> Diff: https://reviews.apache.org/r/32967/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 32975: MESOS-1790 Adds chown option to CommandInfo.URI

[Jim Klucar]

> On April 8, 2015, 9:57 p.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, line 332
> > 
> >
> > and chown is requested.

done


> On April 8, 2015, 9:57 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 208
> > 
> >
> > please update type_utils.cpp to account for this field when comparing 
> > CommandInfos.

done


> On April 8, 2015, 9:57 p.m., Vinod Kone wrote:
> > src/tests/fetcher_tests.cpp, lines 112-116
> > 
> >
> > Add a note/todo here mentioning why you are setting these fields but 
> > not doing any asserts/expects on them.

With the type_utils.cpp change, the EXPECT_EQ on lines 140/141 should be 
checking the chown field. Should I still add text regarding how we can't check 
this capability without assuming the user running the test has permission to 
chown to some other user?


- Jim


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


On April 8, 2015, 2:20 p.m., Jim Klucar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32975/
> ---
> 
> (Updated April 8, 2015, 2:20 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-1790
> https://issues.apache.org/jira/browse/MESOS-1790
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added chown to CommandInfo.URI protocol buffer as an optional
> boolean that defaults to true, the current chown behavior.
> 
> The fetcher was updated to skip the os::chown operation if the chown
> boolean is set to false.
> 
> No documentation was updated.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
> 
> Diff: https://reviews.apache.org/r/32975/diff/
> 
> 
> Testing
> ---
> 
> Unit testing this functionality is difficult because it would require that 
> the user running the test to have permission to chown a file to someone other 
> than themselves. I didn't want to add that as a requirement to build. I added 
> the new field to the existing test cases just to see that they populate.
> 
> 
> Thanks,
> 
> Jim Klucar
> 
>

Re: Review Request 32967: Cleaned upstyle and comments in modules.

[Till Toenshoff]

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



include/mesos/module/authenticatee.hpp


Could you please make this "{}" without that space as suggested by the JIRA?



src/examples/test_isolator_module.cpp


Can you please reduce the change of the comment towards replacing 
"testCpuIsolator" with "org_apache_mesos_TestCpuIsolator" as described by Kapil 
on the JIRA?

My point here is that your other changes do not seem to add value or 
improve readablity.

Same with the others as well, thanks!


- Till Toenshoff


On April 8, 2015, 8:05 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32967/
> ---
> 
> (Updated April 8, 2015, 8:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Kapil Arya, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2565
> https://issues.apache.org/jira/browse/MESOS-2565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed minor style issues in modules and the misleading comments.
> 
> 
> Diffs
> -
> 
>   include/mesos/module/authenticatee.hpp 
> 94de988d524d6390a9f59edbc61f1f7cae514efd 
>   include/mesos/module/authenticator.hpp 
> 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 
>   include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce 
>   include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 
>   src/examples/test_anonymous_module.cpp 
> 859f4e19e0d50794172af4cff708edf171e6d02f 
>   src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 
>   src/examples/test_isolator_module.cpp 
> 2c303f5ff2f17d56a840a5e0108281873c17ce6a 
>   src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de 
> 
> Diff: https://reviews.apache.org/r/32967/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 32967: Cleaned upstyle and comments in modules.

[Alexander Rukletsov]

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



include/mesos/module/authenticatee.hpp


Kill the space between braces. Here and below. Also please correct 
`include/mesos/module.hpp` as well.



src/examples/test_anonymous_module.cpp


Let's keep this comment short as it used to be, just fix the symbol name as 
discussed in MESOS-2565. Same for other entries.


- Alexander Rukletsov


On April 8, 2015, 8:05 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32967/
> ---
> 
> (Updated April 8, 2015, 8:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Kapil Arya, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2565
> https://issues.apache.org/jira/browse/MESOS-2565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed minor style issues in modules and the misleading comments.
> 
> 
> Diffs
> -
> 
>   include/mesos/module/authenticatee.hpp 
> 94de988d524d6390a9f59edbc61f1f7cae514efd 
>   include/mesos/module/authenticator.hpp 
> 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 
>   include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce 
>   include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 
>   src/examples/test_anonymous_module.cpp 
> 859f4e19e0d50794172af4cff708edf171e6d02f 
>   src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 
>   src/examples/test_isolator_module.cpp 
> 2c303f5ff2f17d56a840a5e0108281873c17ce6a 
>   src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de 
> 
> Diff: https://reviews.apache.org/r/32967/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 32967: Cleaned upstyle and comments in modules.

[Alexander Rukletsov]

> On April 8, 2015, 1:20 p.m., Alexander Rukletsov wrote:
> > Please check my comment to https://reviews.apache.org/r/32608/. Also, 
> > please add Kapil Arya , myself  and a committer to the 
> > list of reviewers.
> 
> Aditi Dixit wrote:
> Would adding any committer be ok? Or should I email and ask a committer?

tillt will shepherd this.


- Alexander


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


On April 8, 2015, 8:05 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32967/
> ---
> 
> (Updated April 8, 2015, 8:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Kapil Arya, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2565
> https://issues.apache.org/jira/browse/MESOS-2565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed minor style issues in modules and the misleading comments.
> 
> 
> Diffs
> -
> 
>   include/mesos/module/authenticatee.hpp 
> 94de988d524d6390a9f59edbc61f1f7cae514efd 
>   include/mesos/module/authenticator.hpp 
> 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 
>   include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce 
>   include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 
>   src/examples/test_anonymous_module.cpp 
> 859f4e19e0d50794172af4cff708edf171e6d02f 
>   src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 
>   src/examples/test_isolator_module.cpp 
> 2c303f5ff2f17d56a840a5e0108281873c17ce6a 
>   src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de 
> 
> Diff: https://reviews.apache.org/r/32967/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>