Re: Review Request 36173: Update attributes doc to reflect current supported attributes types.

[Adam B]

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



docs/attributes-resources.md (line 35)


Doesn't `( scalar | range | text | "," )+` look like a set that doesn't 
wrap itself in `{}` braces?
It doesn't look like Attributes::parse handles the "," case, so let's 
remove that (and the `()+`) from the documentation.


- Adam B


On July 3, 2015, 2:30 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36173/
> ---
> 
> (Updated July 3, 2015, 2:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update attributes doc to reflect current supported attributes types.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 0ae8b5908fe0b3a3499e6d813afbb328a13bdcde 
> 
> Diff: https://reviews.apache.org/r/36173/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36173: Update attributes doc to reflect current supported attributes types.

[Timothy Chen]

> On July 6, 2015, 5:25 a.m., Adam B wrote:
> > docs/attributes-resources.md, line 11
> > 
> >
> > s/either supported by Attributes or Resources/supported by Attributes 
> > and Resources/
> > And are sets a 'type' or just a collection of some other type (text)? 
> > Same with ranges, aren't they just a collection of scalars?

We do treat it as a different type since it has a different syntax of how it's 
defined.


- Timothy


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


On July 3, 2015, 9:30 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36173/
> ---
> 
> (Updated July 3, 2015, 9:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update attributes doc to reflect current supported attributes types.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 0ae8b5908fe0b3a3499e6d813afbb328a13bdcde 
> 
> Diff: https://reviews.apache.org/r/36173/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36173: Update attributes doc to reflect current supported attributes types.

[Timothy Chen]

> On July 6, 2015, 5:25 a.m., Adam B wrote:
> > docs/attributes-resources.md, line 31
> > 
> >
> > No sets? Or are attributes implicitly sets?

I was suprised when I read the source code too, but we actually explicitly 
don't support sets for attributes yet.


> On July 6, 2015, 5:25 a.m., Adam B wrote:
> > docs/attributes-resources.md, line 39
> > 
> >
> > 3 types? What about plain text?

Plain text is not a supported resource type. Not sure why but that's what the 
parse code looks like.


- Timothy


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


On July 3, 2015, 9:30 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36173/
> ---
> 
> (Updated July 3, 2015, 9:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update attributes doc to reflect current supported attributes types.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 0ae8b5908fe0b3a3499e6d813afbb328a13bdcde 
> 
> Diff: https://reviews.apache.org/r/36173/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36173: Update attributes doc to reflect current supported attributes types.

[Adam B]

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

Ship it!


Seems like scalar and text are basic types, and then ranges and sets are 
compound types. Sounds weird to talk of 4 types that apply to either, and then 
only name 3 for each.


docs/attributes-resources.md (line 11)


s/either supported by Attributes or Resources/supported by Attributes and 
Resources/
And are sets a 'type' or just a collection of some other type (text)? Same 
with ranges, aren't they just a collection of scalars?



docs/attributes-resources.md (lines 25 - 27)


s/labelString/text/g?



docs/attributes-resources.md (line 31)


No sets? Or are attributes implicitly sets?



docs/attributes-resources.md (line 39)


3 types? What about plain text?


- Adam B


On July 3, 2015, 2:30 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36173/
> ---
> 
> (Updated July 3, 2015, 2:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update attributes doc to reflect current supported attributes types.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 0ae8b5908fe0b3a3499e6d813afbb328a13bdcde 
> 
> Diff: https://reviews.apache.org/r/36173/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.

[Benjamin Hindman]

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

Ship it!


I updated this to be specific to using 'posix' isolation mechanisms and 
committed. I also killed the reference in upgrades.md as asked by AdamB, which 
I agree will probably be weird to maintain there.

Note that there are still other systemd related issues when using 'cgroups' 
isolation mechanisms that need to get worked out but hopefully this will be 
helpful for folks that are using systemd with just 'posix' isolation.

- Benjamin Hindman


On March 27, 2015, 2:09 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32543/
> ---
> 
> (Updated March 27, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Brenden Matthews.
> 
> 
> Bugs: Mesos-2555
> https://issues.apache.org/jira/browse/Mesos-2555
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented the problem and solution encountered in MESOS-2419.
> 
> 
> Diffs
> -
> 
>   docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 
>   docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 
> 
> Diff: https://reviews.apache.org/r/32543/diff/
> 
> 
> Testing
> ---
> 
> markdown check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.

[Benjamin Hindman]

> On March 27, 2015, 9:17 a.m., Adam B wrote:
> > docs/slave-recovery.md, line 71
> > 
> >
> > (If the slave does not come back, each executorDriver shuts itself down 
> > after $MESOS_RECOVERY_TIMEOUT.)
> > 
> > Important question: If an executor is killed, does this systemd mode 
> > affect whether its tasks would get killed?
> 
> Alexander Rukletsov wrote:
> Adam, could you please explain what use case do you have in mind and how 
> it is related to slave recovery?
> 
> Adam B wrote:
> It's not related to slave recovery necessarily, but to how this KillMode 
> impacts other processes like a custom executor. Some frameworks (like HDFS) 
> have a custom executor that launches task(s) as a separate 
> process/subprocess. If the executor is killed (kill -9, or shutdown by the 
> framework/admin), will this change in KillMode affect whether the executors 
> task subprocesses also get killed?
> I'm mostly worried about this KillMode change suddenly leaving stranded 
> task processes if/when executors are killed.
> 
> Alexander Rukletsov wrote:
> I thought that's exactly why we have containerizers: clean-up all 
> stranded processes.
> 
> Adam B wrote:
> Fair enough, when the slave is running. But what if the executor is 
> killed while the slave (thus also the containerizer) is shutdown/recovering?
> I'm not claiming there's anything necessarily wrong with using this 
> KillMode. I just ask the question to make sure we don't recommend a setting 
> that may fix one issue but cause others.
> 
> Alexander Rukletsov wrote:
> I see your point. I would be surprised if this setting will cause the 
> issue, but let's check: better safe than sorry.

The KillMode is only relevant when stopping the "root" process of a systemd 
unit (e.g., via 'systemctl stop'). When another process within the same cgroup 
dies systemd doesn't do anything about it, the normal Linux/init reaping takes 
place. Thus, the suggestion documented in this review is correct. HOWEVER, it 
only applies when using 'posix' isolation since when using 'cgroups' isolation 
the processes are in another cgroup. I updated the documentation accordingly 
before committing.


- Benjamin


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


On March 27, 2015, 2:09 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32543/
> ---
> 
> (Updated March 27, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Brenden Matthews.
> 
> 
> Bugs: Mesos-2555
> https://issues.apache.org/jira/browse/Mesos-2555
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented the problem and solution encountered in MESOS-2419.
> 
> 
> Diffs
> -
> 
>   docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 
>   docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 
> 
> Diff: https://reviews.apache.org/r/32543/diff/
> 
> 
> Testing
> ---
> 
> markdown check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 32384: Adding perf check to configure

[Benjamin Hindman]

> On July 2, 2015, 11:45 a.m., haosdent huang wrote:
> > configure.ac, line 1121
> > 
> >
> > Seems other exist checks use AC_CHECK_TOOL, such as
> > ```
> > AC_CHECK_TOOL([PROTOCOMPILER_TEST], [protoc], [], 
> > [$PROTOBUFPREFIX/bin])
> > ```

FYI, I dropped this issue since we killed the configure.ac parts of this review.


- Benjamin


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


On July 2, 2015, 8:09 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32384/
> ---
> 
> (Updated July 2, 2015, 8:09 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Bugs: MESOS-2166
> https://issues.apache.org/jira/browse/MESOS-2166
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> PerfEventIsolatorTest.ROOT_CGROUPS_Sample requires 'perf' to be installed
> 
> 
> Diffs
> -
> 
>   configure.ac 9290958 
>   src/tests/environment.cpp f111a77 
> 
> Diff: https://reviews.apache.org/r/32384/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 32384: Adding perf check to configure

[Benjamin Hindman]

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

Ship it!


Your check for 'perf' in the configuration is really not super useful as most 
folks will likely just ignore it. The PerfFilter, however, will do the trick 
sufficiently so I'll just clean up the issues I mentioned and get this 
committed, thanks!


src/tests/environment.cpp (line 207)


To be more consistent with the other TestFilter naming, let's do 
s/PerfTest/Perf/.



src/tests/environment.cpp (line 215)


Indentation is off here.



src/tests/environment.cpp (lines 223 - 227)


This is really unnecessary as they won't even get compiled on non-Linux 
systems!



src/tests/environment.cpp (line 233)


Let's add a TODO that captures explicitly marking 'perf' related tests 
using a similar naming mechanism as the other filters, e.g., ROOT_, CGROUPS_, 
etc.


- Benjamin Hindman


On July 2, 2015, 8:09 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32384/
> ---
> 
> (Updated July 2, 2015, 8:09 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Bugs: MESOS-2166
> https://issues.apache.org/jira/browse/MESOS-2166
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> PerfEventIsolatorTest.ROOT_CGROUPS_Sample requires 'perf' to be installed
> 
> 
> Diffs
> -
> 
>   configure.ac 9290958 
>   src/tests/environment.cpp f111a77 
> 
> Diff: https://reviews.apache.org/r/32384/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 35755: Changed fetcher to handle leading whitespace in URLs.

[Benjamin Hindman]

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



src/launcher/fetcher.cpp (line 160)


See https://reviews.apache.org/r/36189, I added a "mode" to 'strings::trim' 
so that you can use it just for prefix or suffix trims too! Thus:

string trimmedSourceUri = strings::trim(sourceUri, strings::PREFIX);

Also, a pattern which we've used in the code base quite a bit is to 
"replace" 'sourceUri' by naming the parameter '_sourceUri' and then doing:

string sourceUri = strings::trim(_sourceUri, strings::PREFIX);

Alternatively, we've taken 'sourceUri' the parameter by value and then just 
done:

sourceUri = strings::trim(sourceUri, strings::PREFIX);



src/launcher/fetcher.cpp 


We've used two newlines between top-level definitions to more easily 
contrast the definitions (here and the rest of this review please).



src/tests/fetcher_tests.cpp (line 281)


What's the benefit of adding the new route? It seems like the extra test 
can still use the existing route (unless I'm missing something) and I'd rather 
not add it and leave people trying to understand why it was added).



src/tests/fetcher_tests.cpp (line 292)


It's not clear to me what adding the process name does here?

Perhaps I'm missing something, but I'd rather not have people reading the 
test trying to understand why we do it here but not other places unless it's 
actually necessary, and if it is, perhaps it needs a comment because it's not 
obvious to me!


- Benjamin Hindman


On June 30, 2015, 11:24 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35755/
> ---
> 
> (Updated June 30, 2015, 11:24 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2862
> https://issues.apache.org/jira/browse/MESOS-2862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixes MESOS-2862
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
>   src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 
> 
> Diff: https://reviews.apache.org/r/35755/diff/
> 
> 
> Testing
> ---
> 
> - make check 
> - added a test case for fetcher
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 36189: Add strings::Mode to strings::trim.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [36189]

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

Error:
 2015-07-06 02:59:44 URL:https://reviews.apache.org/r/36189/diff/raw/ 
[103284/103284] -> "36189.patch" [1]
Successfully applied: Add strings::Mode to strings::trim.

See summary.


Review: https://reviews.apache.org/r/36189
Checking 23 files using filter 
--filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo
Total errors found: 0
ERROR: Commit spanning multiple projects.

Please use separate commits for mesos, libprocess and stout.

Paths grouped by project:
mesos:
  docs/mesos-ssl.md
  include/mesos/mesos.proto
  include/mesos/scheduler/scheduler.proto
  src/Makefile.am
  src/common/protobuf_utils.cpp
  src/common/type_utils.cpp
  src/docker/docker.cpp
  src/examples/event_call_framework.cpp
  src/examples/low_level_scheduler_libprocess.cpp
  src/examples/low_level_scheduler_pthread.cpp
  src/master/master.cpp
  src/master/master.hpp
  src/sched/sched.cpp
  src/scheduler/scheduler.cpp
  src/tests/docker_tests.cpp
  src/tests/event_call_framework_test.sh
  src/tests/examples_tests.cpp
  src/tests/low_level_scheduler_libprocess_test.sh
  src/tests/low_level_scheduler_pthread_test.sh
  src/tests/master_tests.cpp
  src/tests/scheduler_tests.cpp
libprocess:
  3rdparty/libprocess/include/process/firewall.hpp
  3rdparty/libprocess/include/process/http.hpp
  3rdparty/libprocess/src/libevent.cpp
  3rdparty/libprocess/src/libevent_ssl_socket.cpp
  3rdparty/libprocess/src/libevent_ssl_socket.hpp
  3rdparty/libprocess/src/openssl.cpp
  3rdparty/libprocess/src/process.cpp
  3rdparty/libprocess/src/tests/ssl_tests.cpp
stout:
  3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp
  3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp
Failed to commit patch

- Mesos ReviewBot


On July 6, 2015, 2:56 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36189/
> ---
> 
> (Updated July 6, 2015, 2:56 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Vinod Kone.
> 
> 
> Bugs: MESOS-2862
> https://issues.apache.org/jira/browse/MESOS-2862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
> 963029bea989a68a484f7b8b47d29ea5fffeb955 
>   3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
> 9733b2ec5d68605b694210c66144b8d9f8c36467 
>   3rdparty/libprocess/include/process/firewall.hpp 
> 692e065f2744f38035d81c0137760d996a295df6 
>   3rdparty/libprocess/include/process/http.hpp 
> 2f89e7ae430bb7af7bb2616758651e1614232820 
>   3rdparty/libprocess/src/libevent.cpp 
> 67e7501cbd5f7b374cb3037c3483c6d8aa5cc587 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 11c1b70fc5b96cd10f773cceb9f344ff613cc857 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 
>   3rdparty/libprocess/src/openssl.cpp 
> 118ce55d4b0ee1735648552e01ce16920db78d51 
>   3rdparty/libprocess/src/process.cpp 
> 883776a6d87f3f14d04e2d574b0e0baa469af579 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 869ed6572392e3cdcf0c0152bcca4b91130e3c04 
>   docs/mesos-ssl.md bb218492df90c922bc4955daeca5513ba9d18633 
>   include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
>   include/mesos/scheduler/scheduler.proto 
> a027da255563c620fa3d7355ad47aa16d2264f77 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf 
>   src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f 
>   src/examples/event_call_framework.cpp 
> 17fdcac44c0a51293a318ef5184f4d48a461abd9 
>   src/examples/low_level_scheduler_pthread.cpp PRE-CREATION 
>   src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb 
>   src/master/master.cpp c5a4875f0d43c5091ae9a52c6b1d04105dfa3914 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 
>   src/tests/docker_tests.cpp a4a2725c05ae0cb88426c587f7ded0da77154edc 
>   src/tests/event_call_framework_test.sh 
> e42c4e8364076a408b33c894f1f3c6a21e12d1dc 
>   src/tests/examples_tests.cpp 3f56b30d8d5e2c7257f0499

Review Request 36189: Add strings::Mode to strings::trim.

[Benjamin Hindman]

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

Review request for mesos and Artem Harutyunyan.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
963029bea989a68a484f7b8b47d29ea5fffeb955 
  3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
9733b2ec5d68605b694210c66144b8d9f8c36467 
  3rdparty/libprocess/include/process/firewall.hpp 
692e065f2744f38035d81c0137760d996a295df6 
  3rdparty/libprocess/include/process/http.hpp 
2f89e7ae430bb7af7bb2616758651e1614232820 
  3rdparty/libprocess/src/libevent.cpp 67e7501cbd5f7b374cb3037c3483c6d8aa5cc587 
  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
11c1b70fc5b96cd10f773cceb9f344ff613cc857 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 
  3rdparty/libprocess/src/openssl.cpp 118ce55d4b0ee1735648552e01ce16920db78d51 
  3rdparty/libprocess/src/process.cpp 883776a6d87f3f14d04e2d574b0e0baa469af579 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
869ed6572392e3cdcf0c0152bcca4b91130e3c04 
  docs/mesos-ssl.md bb218492df90c922bc4955daeca5513ba9d18633 
  include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
  include/mesos/scheduler/scheduler.proto 
a027da255563c620fa3d7355ad47aa16d2264f77 
  src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
  src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
  src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf 
  src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f 
  src/examples/event_call_framework.cpp 
17fdcac44c0a51293a318ef5184f4d48a461abd9 
  src/examples/low_level_scheduler_pthread.cpp PRE-CREATION 
  src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb 
  src/master/master.cpp c5a4875f0d43c5091ae9a52c6b1d04105dfa3914 
  src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
  src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 
  src/tests/docker_tests.cpp a4a2725c05ae0cb88426c587f7ded0da77154edc 
  src/tests/event_call_framework_test.sh 
e42c4e8364076a408b33c894f1f3c6a21e12d1dc 
  src/tests/examples_tests.cpp 3f56b30d8d5e2c7257f0499dc2f5ea61a7348633 
  src/tests/low_level_scheduler_libprocess_test.sh PRE-CREATION 
  src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 
  src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 36116: MESOS-2965: Add implicit conversion from Path to std::string.

[Benjamin Hindman]

> On July 6, 2015, 2:36 a.m., Benjamin Hindman wrote:
> > Ship It!

Thanks Joseph! It would be great to follow up this with JIRA + reviews that 
eliminate the use of 'value' from a lot of our existing code that uses Path.


- Benjamin


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


On July 6, 2015, 2:34 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36116/
> ---
> 
> (Updated July 6, 2015, 2:34 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2965
> https://issues.apache.org/jira/browse/MESOS-2965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-2965: Add implicit conversion from Path to std::string.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> 3211a0d6b88d06b396678eaf20128698f576d198 
> 
> Diff: https://reviews.apache.org/r/36116/diff/
> 
> 
> Testing
> ---
> 
> One unit test to demonstrate statically-checked conversion of Path to string.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36116: MESOS-2965: Add implicit conversion from Path to std::string.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 6, 2015, 2:34 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36116/
> ---
> 
> (Updated July 6, 2015, 2:34 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2965
> https://issues.apache.org/jira/browse/MESOS-2965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-2965: Add implicit conversion from Path to std::string.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> 3211a0d6b88d06b396678eaf20128698f576d198 
> 
> Diff: https://reviews.apache.org/r/36116/diff/
> 
> 
> Testing
> ---
> 
> One unit test to demonstrate statically-checked conversion of Path to string.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36116: MESOS-2965: Add implicit conversion from Path to std::string.

[Joseph Wu]

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

(Updated July 6, 2015, 2:34 a.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


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


Repository: mesos


Description
---

MESOS-2965: Add implicit conversion from Path to std::string.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
3211a0d6b88d06b396678eaf20128698f576d198 

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


Testing
---

One unit test to demonstrate statically-checked conversion of Path to string.


Thanks,

Joseph Wu

Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

[haosdent huang]

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

(Updated July 6, 2015, 2:25 a.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Return empty task list when current master is not a leader.


Diffs (updated)
-

  src/master/http.cpp 2be613b2e4c913b74c12d0d8f2d0e25da3cd3656 

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


Testing
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang

Re: Review Request 36037: Adding /call endpoint to Master

[Ben Mahler]

> On July 3, 2015, 12:29 a.m., Ben Mahler wrote:
> > I chatted with Isabel on IRC and asked her to break apart this change into 
> > more bite-sized chunks, so that we can do smaller reviews and get things 
> > committed incrementally:
> > 
> > (1) Dummy /call handler on the master.
> > (2) Validation.
> > (3) Partial implementation of Call (i.e. parsing logic).
> > 
> > Each part can have its own tests. She will be discarding this review in 
> > favor of smaller chunks, which we can commit incrementally. :)
> > 
> > I also asked her to:
> > 
> > (a) Punt on the constants and remove master/http_constants.hpp, since these 
> > constants aren't adding value (CLOSE -> "close") for the added indirection, 
> > and our existing code doesn't follow this pattern.
> > (b) Pull out the change to src/tests/mesos.hpp, since it is independent.
> 
> Marco Massenzio wrote:
> All good.
> However, I beg to disagree on this point:
> >(a) Punt on the constants and remove master/http_constants.hpp, since 
> these constants aren't adding value (CLOSE -> "close") for the added 
> indirection, and our existing code doesn't follow this pattern.
> 
> We *do* have a `constants.hpp` (and relative .cpp) file and I do believe 
> it does add value (I, for one, certainly appreciated having it when I did the 
> JSON/ZK change ;) ): for example, I've already seen the string 
> `application/x-protobuf` typed up 10 times in just two reviews: there is 
> value in having an APPLICATION_PROTOBUF constant to:
> 
> - avoid difficult-to-spot bugs to typos (`application/x-prolobuf`) that 
> may only surface at runtime in production;
> - avoid typing the same stuff again and again (especially those of us 
> using modern IDEs can take advantage of code-completion ;) )
> - this is anyway common standard good practice and would allow us to not 
> having to agonize too much in case we need to refactor something (say, at 
> some point we want to use `application/x-protobuf-binary` for whatever reason 
> - there's only one place to do so; sure, this is an unlikely example, but 
> there may be cases where it may not be so far-fetched).
> 
> Also, *not* doing it does not save (I think?) any effort in reviewing 
> and/or committing, so seems very low cost for a potential sizeable payoff.
> 
> Ah, yes, and there's also the fact that hard-coded strings sprinkled all 
> over the code base are hard to maintain - I know, I've had to pick up the 
> pieces at least twice in the last 4 years ;)
> 
> PS - I do agree that defining `const string CLOSE = "close"` may be 
> pushing this one step too far... but I'd like to retain it for those more 
> commonly used strings.

I don't think we're in disagreement, I just want this to be punted so that we 
can think carefully about how to improve 'Request' and 'Response' usage, rather 
than bundling it in this code review. Doing more than one thing in a review 
tends to drag out the review, and I didn't want Isabel to get distracted with 
this.

So let's follow up! In particular, having http constants in 
master/http_constants.hpp is strange because it isn't master specific (we have 
common/http.hpp for ones relevant to many components in mesos, http.hpp for 
libprocess). Also, where possible, I'm hoping to avoid the difficulty in header 
map manipulation entirely by providing typed members (there's a 
[TODO](https://github.com/apache/mesos/blob/0.23.0-rc1/3rdparty/libprocess/include/process/http.hpp#L107)
 which briefly alludes to this). For example, `request.connection` could be an 
enum to capture the possible connection types.


- Ben


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


On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36037/
> ---
> 
> (Updated July 2, 2015, 8:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco 
> Massenzio, and Vinod Kone.
> 
> 
> Bugs: MESOS-2860
> https://issues.apache.org/jira/browse/MESOS-2860
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding a call route with HTTP request header validations
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a064d17 
>   src/master/http.cpp 2be613b 
>   src/master/http_constants.hpp PRE-CREATION 
>   src/master/http_constants.cpp PRE-CREATION 
>   src/master/master.hpp af83d3e 
>   src/master/master.cpp a7486d8 
>   src/master/validation.hpp 469d6f5 
>   src/master/validation.cpp 9d128aa 
>   src/tests/call_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp 9157ac0 
> 
> Diff: https://reviews.apache.org/r/36037/diff/
> 
> 
> Testing
> --

Re: Review Request 36037: Adding /call endpoint to Master

[Anand Mazumdar]

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



src/master/http.cpp (lines 328 - 345)


Let's move these lines into a method like:

Try unmarshal(contentType, body)

since essentially we are just unmarshaling the request here.



src/master/http_constants.cpp (lines 27 - 28)


Let's just keep these 2 constants, as the rest seem too generic as per 
BenM's comments. Also , in my opinion, it would be a good idea to move these in 
src/common/http_constants.hpp/cpp . I see us using the same constants also for 
the executor HTTP API's.


Two more minor suggestions before we decide to break this into smaller chunks.

- Anand Mazumdar


On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36037/
> ---
> 
> (Updated July 2, 2015, 8:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco 
> Massenzio, and Vinod Kone.
> 
> 
> Bugs: MESOS-2860
> https://issues.apache.org/jira/browse/MESOS-2860
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding a call route with HTTP request header validations
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a064d17 
>   src/master/http.cpp 2be613b 
>   src/master/http_constants.hpp PRE-CREATION 
>   src/master/http_constants.cpp PRE-CREATION 
>   src/master/master.hpp af83d3e 
>   src/master/master.cpp a7486d8 
>   src/master/validation.hpp 469d6f5 
>   src/master/validation.cpp 9d128aa 
>   src/tests/call_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp 9157ac0 
> 
> Diff: https://reviews.apache.org/r/36037/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36106]

All tests passed.

- Mesos ReviewBot


On July 5, 2015, 6:30 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> ---
> 
> (Updated July 5, 2015, 6:30 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and 
> Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> cgroups implementation does not have a cpuacct subsystem implementation as of
> today. Adding the implementation for stat function.
> 
> Changes:
>   - added Stats class to encapsulate cpuacct.stat data
>   - added implementation for cpuacct::stats
>   - added unit tests
> 
> Jira: MESOS-2961
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 
>   src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 
>   src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a 
> 
> Diff: https://reviews.apache.org/r/36106/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Jojy Varghese]

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

(Updated July 5, 2015, 6:30 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy 
Chen.


Changes
---

review changes


Repository: mesos


Description
---

cgroups implementation does not have a cpuacct subsystem implementation as of
today. Adding the implementation for stat function.

Changes:
  - added Stats class to encapsulate cpuacct.stat data
  - added implementation for cpuacct::stats
  - added unit tests

Jira: MESOS-2961


Diffs (updated)
-

  src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 
  src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 
  src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Jojy Varghese]

> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 2014-2027
> > 
> >
> > Any reason you're not just re-using cgroups::stat here? I'd suggest 
> > just calling cgroups::stat for now, should simplify this and make it easier 
> > for the reader. :)
> 
> Jojy Varghese wrote:
> The only reason being that the way cpuacct creates Stat should be 
> encapsulated in the cpuacct::Stat. This is the same reason there is a parse 
> method in Stat. But I can change it to use cgroups::stat if absolutely 
> necessary.
> 
> Jojy Varghese wrote:
> Also wanted to highlight the semantics of Stat class to make it clear 
> what the intent is. Stat can only be created from data read from the stats 
> file (cpuacct.stat). Thus creating Stat without the file data should not be 
> allowed. Once the Stat object is created, you can not mutate it.
> 
> Ben Mahler wrote:
> Imagine this only returned the user time, then we would return a 
> Duration, yes? We would not be returning a `Stat` class with one const 'user' 
> member, with a const getter and a static parse method. That would be 
> overkill, right?
> 
> That's the reasoning I have for making this a plain struct. This is the 
> same situation as above, but we need to return more than one value (e.g. 
> `os::UTSInfo`, `os::Memory`, `os::Load`, etc.)
> 
> Jojy Varghese wrote:
> If we were to just return a single value, I would have preferred it to be 
> a rvalue. Duration is a possiblity but not ideal in this situation as we need 
> just the ticks to seconds convertion which is not provided by Duration. 
> Structures like UTSInfo, Memory etc should be non-mutable as they represet a 
> snapshot of data IMHO.
> 
> Ben Mahler wrote:
> Why would the caller care about the ticks conversion? The caller just 
> wants to know the amount of cpu time (i.e. duration) spent in userland, the 
> tick conversion is an implementation detail of reading the file format, which 
> the caller does not want to be burdened with.
> 
> I didn't understand your comment about returning an rvalue, the return 
> value at the callsite is an rvalue inherently until you assign it a name.. 
> and the return value in the function could be an rvalue if the Duration is 
> constructed inline at the end.. something I'm missing here? Assuming that 
> you're referring to the function body, why would you care about returning an 
> rvalue when we have copy elision?
> 
> Back to the point, from the ideological perspective, I agree with you, 
> immutability can be a very nice functional programming principle. However, 
> the perspective I'm coming from is the experiential perspective, one of 
> having worked on and maintained this code base for a few years, and having a 
> keen interest in seeing things remain simple, straightforward, and 
> maintainable. I'd like all of us to be able to come in to any piece of code 
> and be able to read and understand it without unnecessary effort. What I've 
> seen from when we've tried to embrace immutable structs is that the hit to 
> simplicity and straightforwardness is too high. First, one adds 'const' 
> members, which requires a constructor. Then, we run into the difficulty of 
> having no default assignment and/or assignment operator. So, things are made 
> non-const, and to keep const semantics, one has to add const getters to 
> ensure the caller cannot modify the members. Now this is usually ~5x the 
> amount of code as we originally ha
 d with a simple struct. The cognitive burden of understanding the immutable 
'Stat' abstraction is higher because it is inherently more complicated and 
requires more time to read:
> 
> ```
> struct Stat
> {
>   Duration user;
>   Duration system;
> }
> 
> // VS
> 
> class Stat
> {
> public:
>   // Sometimes this is private and there is a factory. 
>   Stat(Duration user, Duration system)
> : user_(user), system_(system) {}
>   
>   // Default construction for use as values in STL maps, etc.
>   Stat() {}
>   
>   Duration user() const
>   {
> return user_;
>   }
> 
>   Duration system() const
>   {
> return system_;
>   }
> 
> private:
>   // Non-const for assignability / default construction.
>   Duration user_;
>   Duration system_;  
> }
> ```
> 
> So while there are indeed situations where immutability is nice (e.g. 
> state::Variable), the cognitive overhead the code reader faces with the more 
> complicated Stat class is not worth it, when we're dealing with simple 
> wrappers of data, IMHO :)

Duration would be an overkill when "userTimeInSecs" is what is the only thing 
required. But i dont have any strong opininions about it. But when we say that 
a good design principle is an overkill, then that is a grey area. Readbili

Re: Review Request 36185: Create pre-launch hook before a docker container launches in slave.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36185]

All tests passed.

- Mesos ReviewBot


On July 5, 2015, 9:14 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36185/
> ---
> 
> (Updated July 5, 2015, 9:14 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2588
> https://issues.apache.org/jira/browse/MESOS-2588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Create pre-launch hook before a docker container launches in slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f 
>   src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc 
>   src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36185/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 36185: Create pre-launch hook before a docker container launches in slave.

[haosdent huang]

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

Review request for mesos and Timothy Chen.


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


Repository: mesos


Description
---

Create pre-launch hook before a docker container launches in slave.


Diffs
-

  include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f 
  src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc 
  src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf 
  src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 

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


Testing
---


Thanks,

haosdent huang