Re: Review Request 36024: Refactored OSNetUri tests for fetcher to avoid code copy/pasting.

[Artem Harutyunyan]

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

(Updated July 9, 2015, 10:20 p.m.)


Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
---

Refactored OSNetUri tests for fetcher to avoid code copy/pasting.


Diffs
-

  src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 

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


Testing
---

# This is a test case refactoring.

$ make check


Thanks,

Artem Harutyunyan

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

[Artem Harutyunyan]

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

(Updated July 9, 2015, 10:19 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 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.

[Chi Zhang]

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



src/tests/perf_tests.cpp (line 40)


un-used.



src/tests/perf_tests.cpp (line 76)


looks like this is 'covered' by the new declartion in the for loop.

here and everywhere.


- Chi Zhang


On July 9, 2015, 11:11 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36380/
> ---
> 
> (Updated July 9, 2015, 11:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test cases for performance monitor support of multiple output versions 
> depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 
> 
> Diff: https://reviews.apache.org/r/36380/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Chi Zhang]

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


Thanks for taking on this for me!


src/linux/perf.cpp (line 449)


Using Try as a function parameter feels a bit non-idiomatic to me, though I 
agree it's less code this way.



src/linux/perf.cpp (line 459)


2 lines above? other places too.



src/linux/perf.cpp (lines 461 - 473)


This is great abstraction; I agree we should put it into stout. (maybe even 
have os::release just return canonicalized linux version?)



src/linux/perf.cpp (lines 475 - 482)


I think using a hashmap as an intermediate type is more flexisible.

Right now, the operation after 'extract' is the same for all versions, but 
we can make it version-dependent too in the future to support e.g. unit, 
running time, etc.


- Chi Zhang


On July 9, 2015, 11:08 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> ---
> 
> (Updated July 9, 2015, 11:08 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output 
> versions depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

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 10, 2015, 1:06 a.m.)


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


Changes
---

review comments @till


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


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 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Anand Mazumdar]

> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > Code looks good!
> > A few general comments (please address them across the entire review - I 
> > stopped making them in every instance):
> > 
> > - no need for leading underscor in argument lists, the private members now 
> > have a trailing one, so no danger of confusion;
> > - make sure you align the `<<` in streaming LOGs (same as everywhere in 
> > Mesos);
> > - please make sure that **every** public method has a sufficient Doxygen 
> > javadoc-formatted documentation;
> > - while we wait for Isabel's http_constants.hpp file to land, please use a 
> > "surrogate" one, which you can then just replace with an `#include` of the 
> > real one
> 
> Ben Mahler wrote:
> I'll shepherd this for Anand, IMO this change needs to go through a 
> higher level review first, otherwise we're likely to go through a ton of 
> iterations (see part (1) of 'Reviewing' here):
> http://mesos.apache.org/documentation/latest/effective-code-reviewing/

Thanks Ben for agreeing to shepherd this. Looking forward to your inputs and 
discussing the change in greater detail.


> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, line 307
> > 
> >
> > while you are at it, do you mind adding javadoc doxy documentation to 
> > this method?
> > what it does, what the @param's are, what does it return; maybe a link 
> > to the design doc...
> > 
> > as much as you feel like, really: like money and beauty, there's no too 
> > much documentation :)

There is already a TODO on the CALL_HELP variable for documenting this better. 
This can easily be pursued as part of that. Do you still want me to pursue this 
as part of this review ?


> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, lines 311-316
> > 
> >
> > unless you know for a fact that none of this will be `None()` you 
> > *must* check, or this will crash Mesos: hence
> > ```
> > if (contentType.isNone()) {
> >   return BadRequest("Content-Type header MUST be specified");
> > } else if (contentType.get() == Constants.APPLICATION_JSON) {
> >   return MediaNotSupported("We do not support JSON request/response 
> > content yet");
> > } else {
> >   ...
> > }
> > ```
> 
> Isabel Jimenez wrote:
> Hi @marco I commented on previous review that this valdiations will be 
> handle in the split of the /call patch. That's why it's missing here. It'll 
> be added with the rebase.
> 
> Marco Massenzio wrote:
> I'm not entirely sure I understand (but I don't really need to): please 
> then add either a TODO to check the checks (so to speak) or an inline comment 
> stating the invariant - but, if so, I would like to see a CHECK_SOME() to 
> make it explicit.
> Otherwise, someone else (yourself in 3 months!) may not know/remember 
> about this and may add redundant checks and/or or remove the others (and 
> leave the condition unchecked).

My bad, I should have added a better TODO instead of the vague one that says 
"Fix logic when we start supporting application/json". I will add a better 
TODO. I was under the impression that all this would be taken care as part of 
the validations patch.


> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, lines 1246-1249
> > 
> >
> > can you please avoid the leading underscore? they seem largely 
> > unnecessary now that we have the trailing ones for the private members

>From the style guide :

- "We prepend constructor and function arguments with a leading underscore to 
avoid ambiguity and / or shadowing:"

- "Prefer trailing underscores for use as member fields (but not required). 
Some trailing underscores are used to distinguish between similar variables in 
the same scope (think prime symbols), but this should be avoided as much as 
possible, including removing existing instances in the code base."

Seems like I am missing something here, I don't find any reason to follow one 
and discard the other i.e. not use both at once. If not, these need to be 
better explained in the style guide. What do you think ?


> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, line 1261
> > 
> >
> > no leading underscore
> > please add doxy for method

The _ was added as it was needed for resolving ambiguity with the already 
declared event variable used in the function.

Also it already has a explanation of what the method does in the .hpp file 
base-class. What would we gain by adding documentation again here ?


> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, lines 1271-1273
> > 

Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36378, 36380]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 11:11 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36380/
> ---
> 
> (Updated July 9, 2015, 11:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test cases for performance monitor support of multiple output versions 
> depending on kernel version.
> 
> 
> Diffs
> -
> 
>   src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 
> 
> Diff: https://reviews.apache.org/r/36380/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Marco Massenzio]

> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, lines 311-316
> > 
> >
> > unless you know for a fact that none of this will be `None()` you 
> > *must* check, or this will crash Mesos: hence
> > ```
> > if (contentType.isNone()) {
> >   return BadRequest("Content-Type header MUST be specified");
> > } else if (contentType.get() == Constants.APPLICATION_JSON) {
> >   return MediaNotSupported("We do not support JSON request/response 
> > content yet");
> > } else {
> >   ...
> > }
> > ```
> 
> Isabel Jimenez wrote:
> Hi @marco I commented on previous review that this valdiations will be 
> handle in the split of the /call patch. That's why it's missing here. It'll 
> be added with the rebase.

I'm not entirely sure I understand (but I don't really need to): please then 
add either a TODO to check the checks (so to speak) or an inline comment 
stating the invariant - but, if so, I would like to see a CHECK_SOME() to make 
it explicit.
Otherwise, someone else (yourself in 3 months!) may not know/remember about 
this and may add redundant checks and/or or remove the others (and leave the 
condition unchecked).


- Marco


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


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> ---
> 
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Marco Massenzio]

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

Ship it!


Ship It!

- Marco Massenzio


On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36360/
> ---
> 
> (Updated July 9, 2015, 10:34 p.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 constants used commonly through the different HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp bbd063d 
>   src/common/http.cpp 73a4de1 
> 
> Diff: https://reviews.apache.org/r/36360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36360]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36360/
> ---
> 
> (Updated July 9, 2015, 10:34 p.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 constants used commonly through the different HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp bbd063d 
>   src/common/http.cpp 73a4de1 
> 
> Diff: https://reviews.apache.org/r/36360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Marco Massenzio]

> On July 9, 2015, 9:32 p.m., Ben Mahler wrote:
> > Can you move this into the existing common/http.hpp, and remove the content 
> > type one? For content type, would rather see a typed member on 
> > Request/Response than constants here, given the other occurrences:
> > 
> > ```
> > ?  mesos git:(master) ? grep -R Content-Type src | grep -v js | grep -v html
> > src/files/files.cpp:  response.headers["Content-Type"] = 
> > "application/octet-stream";
> > src/files/files.cpp:  response.headers["Content-Type"] = 
> > mime::types[extension];
> > src/tests/fault_tolerance_tests.cpp:  "Content-Type",
> > src/tests/files_tests.cpp:  "Content-Type",
> > src/tests/files_tests.cpp:  AWAIT_EXPECT_RESPONSE_HEADER_EQ("image/gif", 
> > "Content-Type", response);
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/metrics_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/metrics_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/repair_tests.cpp:"Content-Type",  
> >   \
> > src/tests/scheduler_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
> > src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
> > src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
> > src/tests/utils.cpp:  response.get().headers.get("Content-Type"));
> > ?  mesos git:(master) ? grep -R Content-Type 3rdparty | grep -v js | grep 
> > -v html
> > 3rdparty/libprocess/examples/example.cpp:
> > response.headers["Content-Type"] = "text/plain";
> > 3rdparty/libprocess/examples/example.cpp:
> > response.headers["Content-Type"] = "text/plain";
> > 3rdparty/libprocess/include/process/http.hpp:  // specify the 
> > 'Content-Type' header, but the 'Content-Length' and
> > 3rdparty/libprocess/include/process/http.hpp:  headers["Content-Type"] 
> > = "text/javascript";
> > 3rdparty/libprocess/include/process/process.hpp:  // '/path/file'. The 
> > 'Content-Type' header of the HTTP response will
> > 3rdparty/libprocess/src/help.cpp:response.headers["Content-Type"] = 
> > "text/x-markdown";
> > 3rdparty/libprocess/src/http.cpp:  // Overwrite Content-Type if necessary.
> > 3rdparty/libprocess/src/http.cpp:headers["Content-Type"] = 
> > contentType.get();
> > 3rdparty/libprocess/src/http.cpp:return Failure("Attempted to do a POST 
> > with a Content-Type but no body");
> > 3rdparty/libprocess/src/http.cpp:return Failure("Attempted to do a POST 
> > with a Content-Type but no body");
> > 3rdparty/libprocess/src/process.cpp:// While the user is expected 
> > to properly set a 'Content-Type'
> > 3rdparty/libprocess/src/process.cpp:// While the user is expected to 
> > properly set a 'Content-Type'
> > 3rdparty/libprocess/src/process.cpp:// Try and determine the 
> > Content-Type from an extension.
> > 3rdparty/libprocess/src/process.cpp:
> > response.headers["Content-Type"] = assets[name].types[extension];
> > 3rdparty/libprocess/src/profiler.cpp:  response.headers["Content-Type"] = 
> > "application/octet-stream";
> > 3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
> > text/plain\r\n"
> > 3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
> > text/plain\r\n"
> > 3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
> > text/plain\r\n"
> > 3rdparty/libprocess/src/tests/http_tests.cpp:  headers["Content-Type"] = 
> > "text/plain";
> > 3rdparty/libprocess/src/tests/http_tests.cpp:  EXPECT_EQ("text/javascript", 
> > response.headers["Content-Type"]);
> > 3rdparty/libprocess/src/tests/system_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > ```
> 
> Isabel Jimenez wrote:
> ok :)

LOL - there's something to be said about hard-coded strings :)
Thanks, Ben!


- Marco


---
Th

Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.

[Paul Brett]

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

Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.


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


Repository: mesos


Description
---

Test cases for performance monitor support of multiple output versions 
depending on kernel version.


Diffs
-

  src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

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

Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.


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


Repository: mesos


Description
---

Refactor Linux Performance monitor to handle changing 'perf stat' output 
versions depending on kernel version.


Diffs
-

  src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 36360: Adding common constants for HTTP API

[Isabel Jimenez]

> On July 9, 2015, 9:32 p.m., Ben Mahler wrote:
> > Can you move this into the existing common/http.hpp, and remove the content 
> > type one? For content type, would rather see a typed member on 
> > Request/Response than constants here, given the other occurrences:
> > 
> > ```
> > ?  mesos git:(master) ? grep -R Content-Type src | grep -v js | grep -v html
> > src/files/files.cpp:  response.headers["Content-Type"] = 
> > "application/octet-stream";
> > src/files/files.cpp:  response.headers["Content-Type"] = 
> > mime::types[extension];
> > src/tests/fault_tolerance_tests.cpp:  "Content-Type",
> > src/tests/files_tests.cpp:  "Content-Type",
> > src/tests/files_tests.cpp:  AWAIT_EXPECT_RESPONSE_HEADER_EQ("image/gif", 
> > "Content-Type", response);
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/master_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/metrics_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/metrics_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/monitor_tests.cpp:  "Content-Type",
> > src/tests/repair_tests.cpp:"Content-Type",  
> >   \
> > src/tests/scheduler_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
> > src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
> > src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
> > src/tests/utils.cpp:  response.get().headers.get("Content-Type"));
> > ?  mesos git:(master) ? grep -R Content-Type 3rdparty | grep -v js | grep 
> > -v html
> > 3rdparty/libprocess/examples/example.cpp:
> > response.headers["Content-Type"] = "text/plain";
> > 3rdparty/libprocess/examples/example.cpp:
> > response.headers["Content-Type"] = "text/plain";
> > 3rdparty/libprocess/include/process/http.hpp:  // specify the 
> > 'Content-Type' header, but the 'Content-Length' and
> > 3rdparty/libprocess/include/process/http.hpp:  headers["Content-Type"] 
> > = "text/javascript";
> > 3rdparty/libprocess/include/process/process.hpp:  // '/path/file'. The 
> > 'Content-Type' header of the HTTP response will
> > 3rdparty/libprocess/src/help.cpp:response.headers["Content-Type"] = 
> > "text/x-markdown";
> > 3rdparty/libprocess/src/http.cpp:  // Overwrite Content-Type if necessary.
> > 3rdparty/libprocess/src/http.cpp:headers["Content-Type"] = 
> > contentType.get();
> > 3rdparty/libprocess/src/http.cpp:return Failure("Attempted to do a POST 
> > with a Content-Type but no body");
> > 3rdparty/libprocess/src/http.cpp:return Failure("Attempted to do a POST 
> > with a Content-Type but no body");
> > 3rdparty/libprocess/src/process.cpp:// While the user is expected 
> > to properly set a 'Content-Type'
> > 3rdparty/libprocess/src/process.cpp:// While the user is expected to 
> > properly set a 'Content-Type'
> > 3rdparty/libprocess/src/process.cpp:// Try and determine the 
> > Content-Type from an extension.
> > 3rdparty/libprocess/src/process.cpp:
> > response.headers["Content-Type"] = assets[name].types[extension];
> > 3rdparty/libprocess/src/profiler.cpp:  response.headers["Content-Type"] = 
> > "application/octet-stream";
> > 3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
> > text/plain\r\n"
> > 3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
> > text/plain\r\n"
> > 3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
> > text/plain\r\n"
> > 3rdparty/libprocess/src/tests/http_tests.cpp:  headers["Content-Type"] = 
> > "text/plain";
> > 3rdparty/libprocess/src/tests/http_tests.cpp:  EXPECT_EQ("text/javascript", 
> > response.headers["Content-Type"]);
> > 3rdparty/libprocess/src/tests/system_tests.cpp:  
> > response.get().headers.get("Content-Type"));
> > ```

ok :)


- Isabel


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

Re: Review Request 36360: Adding common constants for HTTP API

[Isabel Jimenez]

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

(Updated July 9, 2015, 10:34 p.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 constants used commonly through the different HTTP endpoints


Diffs (updated)
-

  src/common/http.hpp bbd063d 
  src/common/http.cpp 73a4de1 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 36197: Documented "how to become a committer".

[Ben Mahler]

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



docs/committers.md (line 11)


No unanimous requirement :)
https://community.apache.org/newcommitter.html


- Ben Mahler


On July 6, 2015, 1:43 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36197/
> ---
> 
> (Updated July 6, 2015, 1:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1825
> https://issues.apache.org/jira/browse/MESOS-1825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new document "committer-candidate-checklist.md" and wrote
> a paragraph about the path to committership in "committers.md".
> 
> 
> Diffs
> -
> 
>   docs/committer-candidate-checklist.md PRE-CREATION 
>   docs/committers.md ca8a6995c5272f3534ab63f95332565dfcaaf5b9 
> 
> Diff: https://reviews.apache.org/r/36197/diff/
> 
> 
> Testing
> ---
> 
> The rendered files can be viewed here:
> 
> https://gist.github.com/bernd-mesos/00de63ae13efec4331be
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Isabel Jimenez]

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



src/master/http.cpp (line 342)


Same here.


- Isabel Jimenez


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> ---
> 
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Isabel Jimenez]

> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, line 317
> > 
> >
> > the error is actually 415 Media Not Supported (I think - please double 
> > check)

Same here


- Isabel


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


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> ---
> 
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Till Toenshoff]

> On July 9, 2015, 7:19 p.m., Ben Mahler wrote:
> >
> 
> Till Toenshoff wrote:
> Thanks for looking into this Ben - we will shortly propose a fix for the 
> current test break on Ubuntu and also most of these nits.

We decided to revert that commit instead for now until we can thorroughly fix 
this RR.


> On July 9, 2015, 7:19 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2815-2819
> > 
> >
> > Any reason we didn't convert os::stat::mtime to return a Time?
> > 
> > The only other user of os::stat::mtime does the same messy conversion:
> > 
> > ```
> > Future Slave::garbageCollect(const string& path)
> > {
> >   Try mtime = os::stat::mtime(path);
> >   if (mtime.isError()) {
> > LOG(ERROR) << "Failed to find the mtime of '" << path
> ><< "': " << mtime.error();
> > return Failure(mtime.error());
> >   }
> > 
> >   // It is unsafe for testing to use unix time directly, we must use
> >   // Time::create to convert into a Time object that reflects the
> >   // possibly advanced state of the libprocess Clock.
> >   Try time = Time::create(mtime.get());
> >   CHECK_SOME(time);
> > ```

One reason could be that os::stat::mtime lives in stout whereas Time lives in 
libprocess, no?


- Till


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


On June 17, 2015, 3:42 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 17, 2015, 3:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When serving a static file, libprocess returns the header `Last-Modified` 
> which is used by browsers to control Cache.
> When a http request arrives containing the header `If-Modified-Since`, a 
> response `304 Not Modified` is returned if the date in the request and the 
> modification time (as returned by doing `stat` in the file) coincide.
> Unit tests added.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> e47cc7afbc8110759edf25a2dc05d09eda25c417 
>   3rdparty/libprocess/src/process.cpp 
> a67a3afdb30d23eb1b265b04ae662f64e874b6c6 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Ben Mahler]

> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > Code looks good!
> > A few general comments (please address them across the entire review - I 
> > stopped making them in every instance):
> > 
> > - no need for leading underscor in argument lists, the private members now 
> > have a trailing one, so no danger of confusion;
> > - make sure you align the `<<` in streaming LOGs (same as everywhere in 
> > Mesos);
> > - please make sure that **every** public method has a sufficient Doxygen 
> > javadoc-formatted documentation;
> > - while we wait for Isabel's http_constants.hpp file to land, please use a 
> > "surrogate" one, which you can then just replace with an `#include` of the 
> > real one

I'll shepherd this for Anand, IMO this change needs to go through a higher 
level review first, otherwise we're likely to go through a ton of iterations 
(see part (1) of 'Reviewing' here):
http://mesos.apache.org/documentation/latest/effective-code-reviewing/


- Ben


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


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> ---
> 
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Isabel Jimenez]

> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, lines 311-316
> > 
> >
> > unless you know for a fact that none of this will be `None()` you 
> > *must* check, or this will crash Mesos: hence
> > ```
> > if (contentType.isNone()) {
> >   return BadRequest("Content-Type header MUST be specified");
> > } else if (contentType.get() == Constants.APPLICATION_JSON) {
> >   return MediaNotSupported("We do not support JSON request/response 
> > content yet");
> > } else {
> >   ...
> > }
> > ```

Hi @marco I commented on previous review that this valdiations will be handle 
in the split of the /call patch. That's why it's missing here. It'll be added 
with the rebase.


- Isabel


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


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> ---
> 
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 36197: Documented "how to become a committer".

[Benjamin Hindman]

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

Ship it!


LGTM Bernd, let's commit this and ammend as necessary in the future. Thanks!

- Benjamin Hindman


On July 6, 2015, 1:43 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36197/
> ---
> 
> (Updated July 6, 2015, 1:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-1825
> https://issues.apache.org/jira/browse/MESOS-1825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new document "committer-candidate-checklist.md" and wrote
> a paragraph about the path to committership in "committers.md".
> 
> 
> Diffs
> -
> 
>   docs/committer-candidate-checklist.md PRE-CREATION 
>   docs/committers.md ca8a6995c5272f3534ab63f95332565dfcaaf5b9 
> 
> Diff: https://reviews.apache.org/r/36197/diff/
> 
> 
> Testing
> ---
> 
> The rendered files can be viewed here:
> 
> https://gist.github.com/bernd-mesos/00de63ae13efec4331be
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Marco Massenzio]

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


Code looks good!
A few general comments (please address them across the entire review - I 
stopped making them in every instance):

- no need for leading underscor in argument lists, the private members now have 
a trailing one, so no danger of confusion;
- make sure you align the `<<` in streaming LOGs (same as everywhere in Mesos);
- please make sure that **every** public method has a sufficient Doxygen 
javadoc-formatted documentation;
- while we wait for Isabel's http_constants.hpp file to land, please use a 
"surrogate" one, which you can then just replace with an `#include` of the real 
one


src/common/protobuf_utils.hpp (lines 78 - 79)


please use javadoc format
also unnecessary semicolon

s/a/an
(eg "an Event")



src/common/protobuf_utils.cpp (line 198)


"an event"
also the indent looks off to me?
(either four spaces or line up with quotes - but do you really need to 
break the string?)



src/master/http.cpp (line 307)


while you are at it, do you mind adding javadoc doxy documentation to this 
method?
what it does, what the @param's are, what does it return; maybe a link to 
the design doc...

as much as you feel like, really: like money and beauty, there's no too 
much documentation :)



src/master/http.cpp (lines 311 - 316)


unless you know for a fact that none of this will be `None()` you *must* 
check, or this will crash Mesos: hence
```
if (contentType.isNone()) {
  return BadRequest("Content-Type header MUST be specified");
} else if (contentType.get() == Constants.APPLICATION_JSON) {
  return MediaNotSupported("We do not support JSON request/response content 
yet");
} else {
  ...
}
```



src/master/http.cpp (line 317)


the error is actually 415 Media Not Supported (I think - please double 
check)



src/master/http.cpp (line 342)


we should return a `BadRequest` here, shouldn't we? or use UNREACHABLE() 
(but that would seem too radical to me: one could crash Mesos with a malformed 
request: yay for DOS :)



src/master/http.cpp (lines 1246 - 1249)


can you please avoid the leading underscore? they seem largely unnecessary 
now that we have the trailing ones for the private members



src/master/http.cpp (line 1261)


no leading underscore
please add doxy for method



src/master/http.cpp (lines 1271 - 1273)


align << (see other code)



src/master/http.cpp (lines 1277 - 1278)


here too



src/master/master.hpp (line 353)


use javadoc instead


- Marco Massenzio


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60c

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36106, 36326]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 8:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> ---
> 
> (Updated July 9, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
> https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 8, 2015, 8:23 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36226/
> ---
> 
> (Updated July 8, 2015, 8:23 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding missing Apache licence header
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 519e38c 
> 
> Diff: https://reviews.apache.org/r/36226/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 9, 2015, 4:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36273/
> ---
> 
> (Updated July 9, 2015, 4:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen-ification of comments in libprocess process headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 59b50af86e059463a01f3c83701bc5fd143d51a4 
> 
> Diff: https://reviews.apache.org/r/36273/diff/
> 
> 
> Testing
> ---
> 
> `doxygen ../Doxyfile` and visually verified the HTML output.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36218: Doxygen styleguide revisions based on conversation from https://reviews.apache.org/r/36193/.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 8, 2015, 8:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36218/
> ---
> 
> (Updated July 8, 2015, 8:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen styleguide revisions based on conversation from 
> https://reviews.apache.org/r/36193/.
> 
> 
> Diffs
> -
> 
>   docs/mesos-doxygen-style-guide.md 72156c72fc40f72740e57d47d0436c60f6d05bd7 
> 
> Diff: https://reviews.apache.org/r/36218/diff/
> 
> 
> Testing
> ---
> 
> Checked rendered markdown.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Paul Brett]

> On July 9, 2015, 2:46 a.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36
> > 
> >
> > Why do we want to rename major/minor/patch to 
> > primary/secondary/tertiary? The former is a widely accepted format.
> 
> Paul Brett wrote:
> g++ defines _GNU_SOURCE (required for libstdc++).  This #includes 
> sysmacros.h which #define major and minor as macros for makedev.
> 
> Ben Mahler wrote:
> If you add major(), minor(), patch() as methods instead, does that bypass 
> the macro issue?
> 
> Paul Brett wrote:
> No, that doesn't work.  At the calling site, x.major() gets expanded to 
> x.gnu_dev_major() by the macro.
> 
> Kapil Arya wrote:
> Can we just `#undef` the macros for this file. AFAIK, we aren't using 
> them in the source code.
> 
> Paul Brett wrote:
> We could but we would be doing that in a header file which would impact 
> everyone who might ever include stout version directly or indirectly.  How 
> about we use getMajor(), getMinor() and getPatch() accessor methods?
> 
> Ben Mahler wrote:
> Per our chat, how about majorVersion(), minorVersion(), patchVersion() 
> with a note for posterity?
> 
> With getMajor(), seems callers might still be likely to call their 
> variable 'major': `unsigned int major = version.getMajor()`

Changed to majorVersion, minorVersion and patchVersion.


- Paul


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


On July 9, 2015, 10:06 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 9, 2015, 10:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Ben Mahler]

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

Ship it!


Thanks for adding the note Paul! Surprised that these are 'int's rather than 
'unsigned int's.

- Ben Mahler


On July 9, 2015, 10:06 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 9, 2015, 10:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Paul Brett]

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

(Updated July 9, 2015, 10:06 p.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
Kapil Arya, and Vinod Kone.


Changes
---

Incorporate review comments.


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


Repository: mesos


Description
---

Expose major, minor and patch components from stout Version

Note: The names major and minor are not available when compiling with GCC 
because it pulls in the sysmacros.h header implicitly which define these for 
makedev.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
8692323d28131cd5706dde0503d49f8f0b0a1aeb 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.

[Ben Mahler]

> On July 9, 2015, 9:26 p.m., Vinod Kone wrote:
> > Can you make sure to test this with a 22.x driver and 23.x master for 
> > completeness?

Good call, did a manual test by modifying the test framework in 0.22.0 to do 
reconciliation against a master off head with this fix, parses the message 
correctly:

```
$ ./src/test-framework --master=10.35.255.108:5050
I0709 22:00:26.220700 14160 sched.cpp:157] Version: 0.22.0
I0709 22:00:26.228389 14202 sched.cpp:254] New master detected at 
master@10.35.255.108:5050
I0709 22:00:26.229192 14202 sched.cpp:264] No credentials provided. Attempting 
to register without authentication
I0709 22:00:26.233379 14210 sched.cpp:448] Framework registered with 
20150709-214514-1828659978-5050-3855-0001
Registered!
Task 1 is in state TASK_LOST
Aborting because task 1 is in unexpected state TASK_LOST with reason 9 from 
source 0 with message 'Reconciliation: Task is unknown'
I0709 22:00:26.235369 14212 sched.cpp:1623] Asked to abort the driver
I0709 22:00:26.235436 14212 sched.cpp:856] Aborting framework 
'20150709-214514-1828659978-5050-3855-0001'
I0709 22:00:26.235656 14160 sched.cpp:1589] Asked to stop the driver
```


- Ben


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


On July 9, 2015, 6:55 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36361/
> ---
> 
> (Updated July 9, 2015, 6:55 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3025
> https://issues.apache.org/jira/browse/MESOS-3025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See MESOS-3025 for details.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 
> 
> Diff: https://reviews.apache.org/r/36361/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Existing test coverage encompasses this.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 23783: Missing Apache headers for libprocess

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 9, 2015, 9:59 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23783/
> ---
> 
> (Updated July 9, 2015, 9:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Dominic Hamon.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding missing Apache licence headers on .cpp, .hpp and Makefiles
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71e15f0 
>   3rdparty/libprocess/configure.ac c197baf 
>   3rdparty/libprocess/examples/example.cpp 3fb4ef5 
>   3rdparty/libprocess/include/Makefile.am d01880c 
>   3rdparty/libprocess/include/process/address.hpp 88946d5 
>   3rdparty/libprocess/include/process/async.hpp 9af3cc0 
>   3rdparty/libprocess/include/process/clock.hpp 8dc7be8 
>   3rdparty/libprocess/include/process/collect.hpp c713c1b 
>   3rdparty/libprocess/include/process/defer.hpp 7c04736 
>   3rdparty/libprocess/include/process/deferred.hpp 3746d69 
>   3rdparty/libprocess/include/process/delay.hpp 29e3532 
>   3rdparty/libprocess/include/process/dispatch.hpp 617fd43 
>   3rdparty/libprocess/include/process/event.hpp ad4a8f4 
>   3rdparty/libprocess/include/process/executor.hpp 157a1d2 
>   3rdparty/libprocess/include/process/filter.hpp aa0c91b 
>   3rdparty/libprocess/include/process/future.hpp a9e765d 
>   3rdparty/libprocess/include/process/gc.hpp e83c636 
>   3rdparty/libprocess/include/process/gmock.hpp 0fd3f8c 
>   3rdparty/libprocess/include/process/gtest.hpp afaed51 
>   3rdparty/libprocess/include/process/help.hpp 07e99f1 
>   3rdparty/libprocess/include/process/http.hpp 0db303a 
>   3rdparty/libprocess/include/process/id.hpp e586937 
>   3rdparty/libprocess/include/process/io.hpp 6388770 
>   3rdparty/libprocess/include/process/latch.hpp 9d010f0 
>   3rdparty/libprocess/include/process/limiter.hpp 444aa1b 
>   3rdparty/libprocess/include/process/logging.hpp 80b1e8f 
>   3rdparty/libprocess/include/process/message.hpp c67c5e1 
>   3rdparty/libprocess/include/process/metrics/counter.hpp f9cab39 
>   3rdparty/libprocess/include/process/metrics/gauge.hpp 7d02cd5 
>   3rdparty/libprocess/include/process/metrics/metric.hpp 616f060 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp aa44992 
>   3rdparty/libprocess/include/process/metrics/timer.hpp 813115a 
>   3rdparty/libprocess/include/process/mime.hpp 0abeac1 
>   3rdparty/libprocess/include/process/mutex.hpp 37ea49a 
>   3rdparty/libprocess/include/process/network.hpp f4192e0 
>   3rdparty/libprocess/include/process/once.hpp 2b81df3 
>   3rdparty/libprocess/include/process/owned.hpp 0541113 
>   3rdparty/libprocess/include/process/pid.hpp e0a9fce 
>   3rdparty/libprocess/include/process/process.hpp 59b50af 
>   3rdparty/libprocess/include/process/profiler.hpp 86050b1 
>   3rdparty/libprocess/include/process/protobuf.hpp 91493de 
>   3rdparty/libprocess/include/process/queue.hpp 5be29db 
>   3rdparty/libprocess/include/process/reap.hpp 5e5051a 
>   3rdparty/libprocess/include/process/run.hpp a0d7286 
>   3rdparty/libprocess/include/process/sequence.hpp d755b34 
>   3rdparty/libprocess/include/process/shared.hpp d80fb7f 
>   3rdparty/libprocess/include/process/socket.hpp 4d95183 
>   3rdparty/libprocess/include/process/statistics.hpp 929fb8d 
>   3rdparty/libprocess/include/process/subprocess.hpp 869e3d9 
>   3rdparty/libprocess/include/process/system.hpp 4fb3c83 
>   3rdparty/libprocess/include/process/time.hpp 6a57b7b 
>   3rdparty/libprocess/include/process/timeout.hpp 5c46d70 
>   3rdparty/libprocess/include/process/timer.hpp e5d71f6 
>   3rdparty/libprocess/include/process/timeseries.hpp ec0ac67 
>   3rdparty/libprocess/src/clock.cpp dd726c1 
>   3rdparty/libprocess/src/config.hpp d5084bf 
>   3rdparty/libprocess/src/decoder.hpp 85ce9e3 
>   3rdparty/libprocess/src/encoder.hpp f53fc0d 
>   3rdparty/libprocess/src/event_loop.hpp af57fe2 
>   3rdparty/libprocess/src/fatal.hpp 34314fd 
>   3rdparty/libprocess/src/fatal.cpp b2934e0 
>   3rdparty/libprocess/src/gate.hpp e5c9379 
>   3rdparty/libprocess/src/http.cpp 095572c 
>   3rdparty/libprocess/src/io.cpp 4944e28 
>   3rdparty/libprocess/src/latch.cpp cba4dcd 
>   3rdparty/libprocess/src/libev.hpp a0a2f49 
>   3rdparty/libprocess/src/libev.cpp 2b8c68d 
>   3rdparty/libprocess/src/libev_poll.cpp 6191be3 
>   3rdparty/libprocess/src/libevent.hpp 47b93f1 
>   3rdparty/libprocess/src/libevent.cpp 67e7501 
>   3rdparty/libprocess/src/libevent_poll.cpp d0b8946 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 11c1b70 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp cecd7af 
>   3rdparty/lib

Re: Review Request 23783: Missing Apache headers for libprocess

[Isabel Jimenez]

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

(Updated July 9, 2015, 9:59 p.m.)


Review request for mesos, Benjamin Hindman and Dominic Hamon.


Repository: mesos


Description
---

Adding missing Apache licence headers on .cpp, .hpp and Makefiles


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 71e15f0 
  3rdparty/libprocess/configure.ac c197baf 
  3rdparty/libprocess/examples/example.cpp 3fb4ef5 
  3rdparty/libprocess/include/Makefile.am d01880c 
  3rdparty/libprocess/include/process/address.hpp 88946d5 
  3rdparty/libprocess/include/process/async.hpp 9af3cc0 
  3rdparty/libprocess/include/process/clock.hpp 8dc7be8 
  3rdparty/libprocess/include/process/collect.hpp c713c1b 
  3rdparty/libprocess/include/process/defer.hpp 7c04736 
  3rdparty/libprocess/include/process/deferred.hpp 3746d69 
  3rdparty/libprocess/include/process/delay.hpp 29e3532 
  3rdparty/libprocess/include/process/dispatch.hpp 617fd43 
  3rdparty/libprocess/include/process/event.hpp ad4a8f4 
  3rdparty/libprocess/include/process/executor.hpp 157a1d2 
  3rdparty/libprocess/include/process/filter.hpp aa0c91b 
  3rdparty/libprocess/include/process/future.hpp a9e765d 
  3rdparty/libprocess/include/process/gc.hpp e83c636 
  3rdparty/libprocess/include/process/gmock.hpp 0fd3f8c 
  3rdparty/libprocess/include/process/gtest.hpp afaed51 
  3rdparty/libprocess/include/process/help.hpp 07e99f1 
  3rdparty/libprocess/include/process/http.hpp 0db303a 
  3rdparty/libprocess/include/process/id.hpp e586937 
  3rdparty/libprocess/include/process/io.hpp 6388770 
  3rdparty/libprocess/include/process/latch.hpp 9d010f0 
  3rdparty/libprocess/include/process/limiter.hpp 444aa1b 
  3rdparty/libprocess/include/process/logging.hpp 80b1e8f 
  3rdparty/libprocess/include/process/message.hpp c67c5e1 
  3rdparty/libprocess/include/process/metrics/counter.hpp f9cab39 
  3rdparty/libprocess/include/process/metrics/gauge.hpp 7d02cd5 
  3rdparty/libprocess/include/process/metrics/metric.hpp 616f060 
  3rdparty/libprocess/include/process/metrics/metrics.hpp aa44992 
  3rdparty/libprocess/include/process/metrics/timer.hpp 813115a 
  3rdparty/libprocess/include/process/mime.hpp 0abeac1 
  3rdparty/libprocess/include/process/mutex.hpp 37ea49a 
  3rdparty/libprocess/include/process/network.hpp f4192e0 
  3rdparty/libprocess/include/process/once.hpp 2b81df3 
  3rdparty/libprocess/include/process/owned.hpp 0541113 
  3rdparty/libprocess/include/process/pid.hpp e0a9fce 
  3rdparty/libprocess/include/process/process.hpp 59b50af 
  3rdparty/libprocess/include/process/profiler.hpp 86050b1 
  3rdparty/libprocess/include/process/protobuf.hpp 91493de 
  3rdparty/libprocess/include/process/queue.hpp 5be29db 
  3rdparty/libprocess/include/process/reap.hpp 5e5051a 
  3rdparty/libprocess/include/process/run.hpp a0d7286 
  3rdparty/libprocess/include/process/sequence.hpp d755b34 
  3rdparty/libprocess/include/process/shared.hpp d80fb7f 
  3rdparty/libprocess/include/process/socket.hpp 4d95183 
  3rdparty/libprocess/include/process/statistics.hpp 929fb8d 
  3rdparty/libprocess/include/process/subprocess.hpp 869e3d9 
  3rdparty/libprocess/include/process/system.hpp 4fb3c83 
  3rdparty/libprocess/include/process/time.hpp 6a57b7b 
  3rdparty/libprocess/include/process/timeout.hpp 5c46d70 
  3rdparty/libprocess/include/process/timer.hpp e5d71f6 
  3rdparty/libprocess/include/process/timeseries.hpp ec0ac67 
  3rdparty/libprocess/src/clock.cpp dd726c1 
  3rdparty/libprocess/src/config.hpp d5084bf 
  3rdparty/libprocess/src/decoder.hpp 85ce9e3 
  3rdparty/libprocess/src/encoder.hpp f53fc0d 
  3rdparty/libprocess/src/event_loop.hpp af57fe2 
  3rdparty/libprocess/src/fatal.hpp 34314fd 
  3rdparty/libprocess/src/fatal.cpp b2934e0 
  3rdparty/libprocess/src/gate.hpp e5c9379 
  3rdparty/libprocess/src/http.cpp 095572c 
  3rdparty/libprocess/src/io.cpp 4944e28 
  3rdparty/libprocess/src/latch.cpp cba4dcd 
  3rdparty/libprocess/src/libev.hpp a0a2f49 
  3rdparty/libprocess/src/libev.cpp 2b8c68d 
  3rdparty/libprocess/src/libev_poll.cpp 6191be3 
  3rdparty/libprocess/src/libevent.hpp 47b93f1 
  3rdparty/libprocess/src/libevent.cpp 67e7501 
  3rdparty/libprocess/src/libevent_poll.cpp d0b8946 
  3rdparty/libprocess/src/libevent_ssl_socket.hpp 11c1b70 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp cecd7af 
  3rdparty/libprocess/src/logging.cpp 9134718 
  3rdparty/libprocess/src/metrics/metrics.cpp a97b613 
  3rdparty/libprocess/src/openssl.hpp 16f3d56 
  3rdparty/libprocess/src/openssl.cpp 118ce55 
  3rdparty/libprocess/src/openssl_util.hpp f855e27 
  3rdparty/libprocess/src/openssl_util.cpp cd38f17 
  3rdparty/libprocess/src/pid.cpp 979c370 
  3rdparty/libprocess/src/poll_socket.hpp a14d63f 
  3rdparty/libprocess/src/poll_socket.cpp 9cb4ef9 
  3rdparty/libprocess/src/process.cpp 9037b98 
  3rdparty/libprocess/src/proc

Re: Review Request 23784: Missing Apache headers for stout

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 8, 2015, 8:51 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23784/
> ---
> 
> (Updated July 8, 2015, 8:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Dominic Hamon.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding missing Apache licence headers on .cpp, .hpp and Makefiles
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 9df5de4 
>   3rdparty/libprocess/3rdparty/stout/configure.ac 1988d19 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am ff6fb28 
>   3rdparty/libprocess/3rdparty/stout/tests/bits_tests.cpp c47bd13 
>   3rdparty/libprocess/3rdparty/stout/tests/bytes_tests.cpp 1fe7e08 
>   3rdparty/libprocess/3rdparty/stout/tests/cache_tests.cpp f8d6e42 
>   3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 724c5fe 
>   3rdparty/libprocess/3rdparty/stout/tests/error_tests.cpp 35a62b1 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 2a6f67b 
>   3rdparty/libprocess/3rdparty/stout/tests/gzip_tests.cpp 2211f31 
>   3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp 4a8176b 
>   3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp 97a7167 
>   3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp 1fa8fc1 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp c50480b 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 0011f08 
>   3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp 0644d99 
>   3rdparty/libprocess/3rdparty/stout/tests/mac_tests.cpp b80fff9 
>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp f5f20ee 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 11f3bf7 
>   3rdparty/libprocess/3rdparty/stout/tests/none_tests.cpp 1c1f8be 
>   3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp 4a0f60b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp fee942c 
>   3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 6d2d3d5 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp d7d4552 
>   3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp dd3700d 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp b3ce131 
>   3rdparty/libprocess/3rdparty/stout/tests/set_tests.cpp b3cd5f8 
>   3rdparty/libprocess/3rdparty/stout/tests/some_tests.cpp ed48279 
>   3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 9733b2e 
>   3rdparty/libprocess/3rdparty/stout/tests/thread_tests.cpp 6871e8b 
>   3rdparty/libprocess/3rdparty/stout/tests/uuid_tests.cpp 7ac8fc0 
> 
> Diff: https://reviews.apache.org/r/23784/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Ben Mahler]

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


Can you move this into the existing common/http.hpp, and remove the content 
type one? For content type, would rather see a typed member on Request/Response 
than constants here, given the other occurrences:

```
➜  mesos git:(master) ✗ grep -R Content-Type src | grep -v js | grep -v html
src/files/files.cpp:  response.headers["Content-Type"] = 
"application/octet-stream";
src/files/files.cpp:  response.headers["Content-Type"] = 
mime::types[extension];
src/tests/fault_tolerance_tests.cpp:  "Content-Type",
src/tests/files_tests.cpp:  "Content-Type",
src/tests/files_tests.cpp:  AWAIT_EXPECT_RESPONSE_HEADER_EQ("image/gif", 
"Content-Type", response);
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/master_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/metrics_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/metrics_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/monitor_tests.cpp:  "Content-Type",
src/tests/monitor_tests.cpp:  "Content-Type",
src/tests/monitor_tests.cpp:  "Content-Type",
src/tests/monitor_tests.cpp:  "Content-Type",
src/tests/repair_tests.cpp:"Content-Type",  
  \
src/tests/scheduler_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/slave_tests.cpp:  response.get().headers.get("Content-Type"));
src/tests/utils.cpp:  response.get().headers.get("Content-Type"));
➜  mesos git:(master) ✗ grep -R Content-Type 3rdparty | grep -v js | grep -v 
html
3rdparty/libprocess/examples/example.cpp:response.headers["Content-Type"] = 
"text/plain";
3rdparty/libprocess/examples/example.cpp:
response.headers["Content-Type"] = "text/plain";
3rdparty/libprocess/include/process/http.hpp:  // specify the 'Content-Type' 
header, but the 'Content-Length' and
3rdparty/libprocess/include/process/http.hpp:  headers["Content-Type"] = 
"text/javascript";
3rdparty/libprocess/include/process/process.hpp:  // '/path/file'. The 
'Content-Type' header of the HTTP response will
3rdparty/libprocess/src/help.cpp:response.headers["Content-Type"] = 
"text/x-markdown";
3rdparty/libprocess/src/http.cpp:  // Overwrite Content-Type if necessary.
3rdparty/libprocess/src/http.cpp:headers["Content-Type"] = 
contentType.get();
3rdparty/libprocess/src/http.cpp:return Failure("Attempted to do a POST 
with a Content-Type but no body");
3rdparty/libprocess/src/http.cpp:return Failure("Attempted to do a POST 
with a Content-Type but no body");
3rdparty/libprocess/src/process.cpp:// While the user is expected to 
properly set a 'Content-Type'
3rdparty/libprocess/src/process.cpp:// While the user is expected to 
properly set a 'Content-Type'
3rdparty/libprocess/src/process.cpp:// Try and determine the Content-Type 
from an extension.
3rdparty/libprocess/src/process.cpp:response.headers["Content-Type"] = 
assets[name].types[extension];
3rdparty/libprocess/src/profiler.cpp:  response.headers["Content-Type"] = 
"application/octet-stream";
3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
text/plain\r\n"
3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
text/plain\r\n"
3rdparty/libprocess/src/tests/decoder_tests.cpp:"Content-Type: 
text/plain\r\n"
3rdparty/libprocess/src/tests/http_tests.cpp:  headers["Content-Type"] = 
"text/plain";
3rdparty/libprocess/src/tests/http_tests.cpp:  EXPECT_EQ("text/javascript", 
response.headers["Content-Type"]);
3rdparty/libprocess/src/tests/system_tests.cpp:  
response.get().headers.get("Content-Type"));
```

- Ben Mahler


On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36360/
> ---
> 
> (Updated July 9, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjami

Re: Review Request 36360: Adding common constants for HTTP API

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36360]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36360/
> ---
> 
> (Updated July 9, 2015, 9:07 p.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 constants used commonly through the different HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e5b5d36 
>   src/common/http_constants.hpp PRE-CREATION 
>   src/common/http_constants.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.

[Vinod Kone]

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

Ship it!


Can you make sure to test this with a 22.x driver and 23.x master for 
completeness?

- Vinod Kone


On July 9, 2015, 6:55 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36361/
> ---
> 
> (Updated July 9, 2015, 6:55 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3025
> https://issues.apache.org/jira/browse/MESOS-3025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See MESOS-3025 for details.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 
> 
> Diff: https://reviews.apache.org/r/36361/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Existing test coverage encompasses this.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Ben Mahler]

> On July 9, 2015, 2:46 a.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36
> > 
> >
> > Why do we want to rename major/minor/patch to 
> > primary/secondary/tertiary? The former is a widely accepted format.
> 
> Paul Brett wrote:
> g++ defines _GNU_SOURCE (required for libstdc++).  This #includes 
> sysmacros.h which #define major and minor as macros for makedev.
> 
> Ben Mahler wrote:
> If you add major(), minor(), patch() as methods instead, does that bypass 
> the macro issue?
> 
> Paul Brett wrote:
> No, that doesn't work.  At the calling site, x.major() gets expanded to 
> x.gnu_dev_major() by the macro.
> 
> Kapil Arya wrote:
> Can we just `#undef` the macros for this file. AFAIK, we aren't using 
> them in the source code.
> 
> Paul Brett wrote:
> We could but we would be doing that in a header file which would impact 
> everyone who might ever include stout version directly or indirectly.  How 
> about we use getMajor(), getMinor() and getPatch() accessor methods?

Per our chat, how about majorVersion(), minorVersion(), patchVersion() with a 
note for posterity?

With getMajor(), seems callers might still be likely to call their variable 
'major': `unsigned int major = version.getMajor()`


- Ben


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


On July 8, 2015, 10:50 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 8, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Paul Brett]

> On July 9, 2015, 2:46 a.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36
> > 
> >
> > Why do we want to rename major/minor/patch to 
> > primary/secondary/tertiary? The former is a widely accepted format.
> 
> Paul Brett wrote:
> g++ defines _GNU_SOURCE (required for libstdc++).  This #includes 
> sysmacros.h which #define major and minor as macros for makedev.
> 
> Ben Mahler wrote:
> If you add major(), minor(), patch() as methods instead, does that bypass 
> the macro issue?
> 
> Paul Brett wrote:
> No, that doesn't work.  At the calling site, x.major() gets expanded to 
> x.gnu_dev_major() by the macro.
> 
> Kapil Arya wrote:
> Can we just `#undef` the macros for this file. AFAIK, we aren't using 
> them in the source code.

We could but we would be doing that in a header file which would impact 
everyone who might ever include stout version directly or indirectly.  How 
about we use getMajor(), getMinor() and getPatch() accessor methods?


- Paul


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


On July 8, 2015, 10:50 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 8, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Till Toenshoff]

> On July 9, 2015, 7:19 p.m., Ben Mahler wrote:
> >

Thanks for looking into this Ben - we will shortly propose a fix for the 
current test break on Ubuntu and also most of these nits.


- Till


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


On June 17, 2015, 3:42 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 17, 2015, 3:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When serving a static file, libprocess returns the header `Last-Modified` 
> which is used by browsers to control Cache.
> When a http request arrives containing the header `If-Modified-Since`, a 
> response `304 Not Modified` is returned if the date in the request and the 
> modification time (as returned by doing `stat` in the file) coincide.
> Unit tests added.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> e47cc7afbc8110759edf25a2dc05d09eda25c417 
>   3rdparty/libprocess/src/process.cpp 
> a67a3afdb30d23eb1b265b04ae662f64e874b6c6 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Isabel Jimenez]

> On July 9, 2015, 8:23 p.m., Anand Mazumdar wrote:
> > src/common/http_constants.cpp, line 26
> > 
> >
> > minor nit-pick , might consider using std::string; before-hand ?

:) done


- Isabel


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


On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36360/
> ---
> 
> (Updated July 9, 2015, 9:07 p.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 constants used commonly through the different HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e5b5d36 
>   src/common/http_constants.hpp PRE-CREATION 
>   src/common/http_constants.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Isabel Jimenez]

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

(Updated July 9, 2015, 9:07 p.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 constants used commonly through the different HTTP endpoints


Diffs (updated)
-

  src/Makefile.am e5b5d36 
  src/common/http_constants.hpp PRE-CREATION 
  src/common/http_constants.cpp PRE-CREATION 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Jojy Varghese]

> On July 9, 2015, 7:36 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 438-447
> > 
> >
> > What is the plan for introducing javadoc comments? cgroups.hpp seems 
> > like a reasonable candidate, but we should avoid inconsistent comment 
> > formatting within a file :(
> > 
> > I'd propose commenting in the existing style, and following up by doing 
> > a javadoc formatting sweep across the file, if you're interested. Sound 
> > good?

I was explicitly asked to do this way for now (see above reviews). I can remove 
it but will be conflicting with the other reviewer.


> On July 9, 2015, 7:36 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 450-455
> > 
> >
> > Why are we documenting the cpuacct.stat file format here? The caller 
> > should only cares about the user and system times, not the format of the 
> > underlying file :)

I thought extra information about where that data come from will help.


> On July 9, 2015, 7:36 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 458-466
> > 
> >
> > Are these kinds of comments providing any value?

Just for serving doxygen. Not sure what else I could have added. Sugestions are 
welcome.


- Jojy


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


On July 7, 2015, 12:26 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> ---
> 
> (Updated July 7, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2961
> https://issues.apache.org/jira/browse/MESOS-2961
> 
> 
> 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 36336: Expose major, minor and patch components from stout Version

[Kapil Arya]

> On July 8, 2015, 10:46 p.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36
> > 
> >
> > Why do we want to rename major/minor/patch to 
> > primary/secondary/tertiary? The former is a widely accepted format.
> 
> Paul Brett wrote:
> g++ defines _GNU_SOURCE (required for libstdc++).  This #includes 
> sysmacros.h which #define major and minor as macros for makedev.
> 
> Ben Mahler wrote:
> If you add major(), minor(), patch() as methods instead, does that bypass 
> the macro issue?
> 
> Paul Brett wrote:
> No, that doesn't work.  At the calling site, x.major() gets expanded to 
> x.gnu_dev_major() by the macro.

Can we just `#undef` the macros for this file. AFAIK, we aren't using them in 
the source code.


- Kapil


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


On July 8, 2015, 6:50 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 8, 2015, 6:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Jojy Varghese]

> On July 8, 2015, 9:57 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.hpp, line 230
> > 
> >
> > I dont' think we need to expose this.

Since the method is a bit long, kept it but replaced other "usage" methods with 
lamdas.


> On July 8, 2015, 9:57 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1239
> > 
> >
> > It's probably not going to be used outside of usage, so I think it's 
> > safe to inline this in _usage, and if you do want a seperate method this 
> > probably don't need to be exposed too.

same comments as above.


- Jojy


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


On July 9, 2015, 8:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> ---
> 
> (Updated July 9, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
> https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36326: containerizer: added cgroups based statistics.

[Jojy Varghese]

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

(Updated July 9, 2015, 8:38 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

review comments @tim


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


Repository: mesos


Description
---

WIP

Adding cgroups cpustat and memory statistics as preferred way to get usage
statistics for containerizers.

Jira: MESOS-2951


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
  src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36361]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 6:55 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36361/
> ---
> 
> (Updated July 9, 2015, 6:55 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3025
> https://issues.apache.org/jira/browse/MESOS-3025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See MESOS-3025 for details.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 
> 
> Diff: https://reviews.apache.org/r/36361/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Existing test coverage encompasses this.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36360: Adding common constants for HTTP API

[Anand Mazumdar]

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

Ship it!


LGTM, Just some minor comments.


src/common/http_constants.hpp (line 18)


This is not a self-contained header file that we encourage everyone to 
build up, kindly do #include  and remove the adjacent include from the 
cpp file.



src/common/http_constants.hpp (line 26)


s/Supported Content-Tye and Accept headers/Supported media types for 
Content-Type and Accept headers.



src/common/http_constants.cpp (line 26)


minor nit-pick , might consider using std::string; before-hand ?


- Anand Mazumdar


On July 9, 2015, 7:58 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36360/
> ---
> 
> (Updated July 9, 2015, 7:58 p.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 constants used commonly through the different HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e5b5d36 
>   src/common/http_constants.hpp PRE-CREATION 
>   src/common/http_constants.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Paul Brett]

> On July 9, 2015, 2:46 a.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36
> > 
> >
> > Why do we want to rename major/minor/patch to 
> > primary/secondary/tertiary? The former is a widely accepted format.
> 
> Paul Brett wrote:
> g++ defines _GNU_SOURCE (required for libstdc++).  This #includes 
> sysmacros.h which #define major and minor as macros for makedev.
> 
> Ben Mahler wrote:
> If you add major(), minor(), patch() as methods instead, does that bypass 
> the macro issue?

No, that doesn't work.  At the calling site, x.major() gets expanded to 
x.gnu_dev_major() by the macro.


- Paul


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


On July 8, 2015, 10:50 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 8, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Review Request 36360: Adding common constants for HTTP API

[Isabel Jimenez]

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

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 constants used commonly through the different HTTP endpoints


Diffs
-

  src/Makefile.am e5b5d36 
  src/common/http_constants.hpp PRE-CREATION 
  src/common/http_constants.cpp PRE-CREATION 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 36328: Change Request accepts to acceptsEncoding

[Ben Mahler]

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

Ship it!


Ship It!

- Ben Mahler


On July 8, 2015, 8:08 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36328/
> ---
> 
> (Updated July 8, 2015, 8:08 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2860
> https://issues.apache.org/jira/browse/MESOS-2860
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> accepts Request method implement 'Accepts-Encoding', we should change it's 
> name to better reflect this.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 8d9adc5 
>   3rdparty/libprocess/src/encoder.hpp b898658 
>   3rdparty/libprocess/src/http.cpp 0898335 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9fccab1 
> 
> Diff: https://reviews.apache.org/r/36328/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36314: Added test for passing total slave's resources in ResourceUsage.

[Jie Yu]

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

Ship it!


LGTM! Some nits.


src/tests/slave_tests.cpp (line 2258)


I don't think you need to use the MockExecutor here.



src/tests/slave_tests.cpp (line 2262)


No need to use the temp variable. I would suggest the following:

```
slave::Flags flags = CreateSlaveFlage();
flags.resources = "cpus:2;mem:1024;disk:1024;ports:[31000-32000]";

...

AWAIT_READY(usage);

EXPECT_EQ(Resources(usage.get().total()),
  Resources::parse(flags.resources).get());
```



src/tests/slave_tests.cpp (line 2280)


This check seems to be redundent. I would suggest killing it.



src/tests/slave_tests.cpp (line 2301)


Same. No need for MockExecutor.



src/tests/slave_tests.cpp (lines 2305 - 2307)


No need for this temp variable.


- Jie Yu


On July 9, 2015, 1:09 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36314/
> ---
> 
> (Updated July 9, 2015, 1:09 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up tests for https://reviews.apache.org/r/36204/
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp ea0ddf38444915a1cda71cce6a8897803fa49198 
> 
> Diff: https://reviews.apache.org/r/36314/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Ben Mahler]

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



src/linux/cgroups.hpp (lines 438 - 447)


What is the plan for introducing javadoc comments? cgroups.hpp seems like a 
reasonable candidate, but we should avoid inconsistent comment formatting 
within a file :(

I'd propose commenting in the existing style, and following up by doing a 
javadoc formatting sweep across the file, if you're interested. Sound good?



src/linux/cgroups.hpp (lines 450 - 455)


Why are we documenting the cpuacct.stat file format here? The caller should 
only cares about the user and system times, not the format of the underlying 
file :)



src/linux/cgroups.hpp (lines 458 - 466)


Are these kinds of comments providing any value?


- Ben Mahler


On July 7, 2015, 12:26 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> ---
> 
> (Updated July 7, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2961
> https://issues.apache.org/jira/browse/MESOS-2961
> 
> 
> 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 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36318]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> ---
> 
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Ben Mahler]

> On July 9, 2015, 2:46 a.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36
> > 
> >
> > Why do we want to rename major/minor/patch to 
> > primary/secondary/tertiary? The former is a widely accepted format.
> 
> Paul Brett wrote:
> g++ defines _GNU_SOURCE (required for libstdc++).  This #includes 
> sysmacros.h which #define major and minor as macros for makedev.

If you add major(), minor(), patch() as methods instead, does that bypass the 
macro issue?


- Ben


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


On July 8, 2015, 10:50 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 8, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Ben Mahler]

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



3rdparty/libprocess/src/process.cpp (lines 2815 - 2819)


Any reason we didn't convert os::stat::mtime to return a Time?

The only other user of os::stat::mtime does the same messy conversion:

```
Future Slave::garbageCollect(const string& path)
{
  Try mtime = os::stat::mtime(path);
  if (mtime.isError()) {
LOG(ERROR) << "Failed to find the mtime of '" << path
   << "': " << mtime.error();
return Failure(mtime.error());
  }

  // It is unsafe for testing to use unix time directly, we must use
  // Time::create to convert into a Time object that reflects the
  // possibly advanced state of the libprocess Clock.
  Try time = Time::create(mtime.get());
  CHECK_SOME(time);
```



3rdparty/libprocess/src/process.cpp (line 2827)


Shouldn't this say if the file hasn't been modified since the requested 
time? Note that a < comparison becomes possible when dealing with Time rather 
than time_t.



3rdparty/libprocess/src/process.cpp (line 2833)


Why did you need the {} here?



3rdparty/libprocess/src/process.cpp (lines 2839 - 2846)


Any reason to prefer -1 special cases here to just using Try and 
handling errors?



3rdparty/libprocess/src/process.cpp (lines 2852 - 2859)


Why isn't this in the `if (modified)` branch below?



3rdparty/libprocess/src/process.cpp (lines 2857 - 2858)


Mind lining this up according to the style guide and the code around you?



3rdparty/libprocess/src/tests/process_tests.cpp (line 1680)


How about s/cache/HttpCache/ ?

We should probably start pulling out http server tests.



3rdparty/libprocess/src/tests/process_tests.cpp (line 1711)


std::string here but string above? Mind removing the std:: qualifiers?



3rdparty/libprocess/src/tests/process_tests.cpp (lines 1726 - 1743)


Yikes, could we add library support for parsing into Time to clean this up? 
Looks like it could be a lot cleaner.



3rdparty/libprocess/src/tests/process_tests.cpp (lines 1747 - 1751)


Mind just using AWAIT_EXPECT_RESPONSE_STATUS_EQ to clean this up a bit? 
Ditto for the other cases here.


- Ben Mahler


On June 17, 2015, 3:42 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 17, 2015, 3:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When serving a static file, libprocess returns the header `Last-Modified` 
> which is used by browsers to control Cache.
> When a http request arrives containing the header `If-Modified-Since`, a 
> response `304 Not Modified` is returned if the date in the request and the 
> modification time (as returned by doing `stat` in the file) coincide.
> Unit tests added.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> e47cc7afbc8110759edf25a2dc05d09eda25c417 
>   3rdparty/libprocess/src/process.cpp 
> a67a3afdb30d23eb1b265b04ae662f64e874b6c6 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 34139: AppC image discovery.

[Vinod Kone]

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


tests!?


src/slave/containerizer/provisioners/appc/discovery.hpp (line 36)


This class and methods all need commenting!



src/slave/containerizer/provisioners/appc/discovery.hpp (line 50)


comments.



src/slave/containerizer/provisioners/appc/discovery.hpp (line 68)


comments.



src/slave/containerizer/provisioners/appc/discovery.cpp (line 60)


+1

please make sure that code in a review only depends on stuff in the current 
review and its dependent reviews. it's hard to review o.w.


- Vinod Kone


On July 7, 2015, 7:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34139/
> ---
> 
> (Updated July 7, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Local and simple discovery only.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34138: AppC hash computation.

[Vinod Kone]

> On July 9, 2015, 6:47 p.m., Vinod Kone wrote:
> >

Also, we need tests!?


- Vinod


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


On July 7, 2015, 7:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34138/
> ---
> 
> (Updated July 7, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppC hash computation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 36361: Ensure StatusUpdate.uuid is set for backwards compatiblity with 0.22.x scheduler drivers.

[Ben Mahler]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

See MESOS-3025 for details.


Diffs
-

  src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
  src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
  src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
  src/scheduler/scheduler.cpp d5ac04cb4549e5ef886ff3c01fff414083a63c06 

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


Testing
---

make check

Existing test coverage encompasses this.


Thanks,

Ben Mahler

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Anand Mazumdar]

> On July 8, 2015, 6:07 p.m., Isabel Jimenez wrote:
> > src/master/http.cpp, line 316
> > 
> >
> > Al the json logic will be handled in the split of 36037 this lines 
> > aren't very useful here

Left the TODO for now as a place-holder but removed the parsing logic. Not 
exactly the way you wanted it, but I am expecting this to go away anyways soon.


> On July 8, 2015, 6:07 p.m., Isabel Jimenez wrote:
> > src/master/master.hpp, line 1584
> > 
> >
> > We should leave this empty line

My bad.


- Anand


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


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> ---
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> 
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> ---
> 
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events

[Anand Mazumdar]

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

(Updated July 9, 2015, 6:49 p.m.)


Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
Vinod Kone.


Changes
---

Addressed review comments + added a fix to close the pipe upon exit. Would add 
a test-case for it in a separate review when submitting the 
re-registration(...) change thereby testing for failover etc.


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


Repository: mesos


Description
---

This change lays the ground-work for the master's ability to stream events back 
to the client. This review turned out a bit too large for my own liking but in 
a nutshell, it just takes a subscribe request and puts a subscribed event back 
on the stream.

Explanation of changes:
- Made a generic FrameworkDriver interface that the master now uses to 
communicate with the frameworks instead of just invoking 
send(framework->pid,...)
- Framework's can be of 2 types now http, libprocess.
- Made a adapter class that takes the messages from master, transforms them to 
events that the http framework driver can then understand.
- This still uses hard-coded http related constants. They can go away when 
Isabel submits her validation change (36217)
- This change prefers use of using trailing under-scores as member variables 
from the style guide.


Diffs (updated)
-

  src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
  src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
  src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
  src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
  src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
  src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 

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


Testing
---

make check + a simple test for subscribe call received a subscribed event back 
on the stream.


Thanks,

Anand Mazumdar

Re: Review Request 34138: AppC hash computation.

[Vinod Kone]

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



src/slave/containerizer/provisioners/appc/hash.hpp (line 40)


A class with only static methods seems weird. Why not put this in a 
namespace instead?

Also, why is this defined here and not in libprocess?



src/slave/containerizer/provisioners/appc/hash.hpp (line 61)


This should take a 'Path' type instead of 'string'.



src/slave/containerizer/provisioners/appc/hash.hpp (line 69)


looks like 'command()' is only used once here. why split it into a function?


- Vinod Kone


On July 7, 2015, 7:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34138/
> ---
> 
> (Updated July 7, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppC hash computation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36273]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 4:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36273/
> ---
> 
> (Updated July 9, 2015, 4:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen-ification of comments in libprocess process headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 59b50af86e059463a01f3c83701bc5fd143d51a4 
> 
> Diff: https://reviews.apache.org/r/36273/diff/
> 
> 
> Testing
> ---
> 
> `doxygen ../Doxyfile` and visually verified the HTML output.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Till Toenshoff]

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



src/tests/cgroups_tests.cpp (line 1191)


Could you please add one or two lines of description for this test as a 
comment above the test itself?


- Till Toenshoff


On July 7, 2015, 12:26 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> ---
> 
> (Updated July 7, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2961
> https://issues.apache.org/jira/browse/MESOS-2961
> 
> 
> 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 35687: Added capabilities to state.json

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35687]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> ---
> 
> (Updated July 9, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Marco Massenzio and Vinod Kone.
> 
> 
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added capabilities to state.json and test for the same
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 
>   src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22 
> 
> Diff: https://reviews.apache.org/r/35687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Till Toenshoff]

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



src/linux/cgroups.hpp (line 445)


1 space after @return and before "Some", no?



src/linux/cgroups.hpp (lines 473 - 476)


You seem to be using variable space identation for this block unlike the 
one for the 'cgroup()'. Our styleguide seems to hint to use fixed indentation 
for these doxygen headers.


- Till Toenshoff


On July 7, 2015, 12:26 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> ---
> 
> (Updated July 7, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2961
> https://issues.apache.org/jira/browse/MESOS-2961
> 
> 
> 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 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Michael Park]

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

(Updated July 9, 2015, 5:07 p.m.)


Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till Toenshoff.


Changes
---

Remove unicode characters copy/pasted from `reviewboardrc` documentation.


Repository: mesos


Description (updated)
---

Handles the case where the ReviewBoard URL is invalid.

```
af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
(29 seconds ago)

Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
```

Reads `REVIEWBOARD_URL` from `.reviewboardrc` if it's available. We use 
`imp.load_source` to import the `.reviewboardrc` file.
> __.reviewboardrc__
>
> The .reviewboardrc file is a generic place for configuring a repository. This 
> must be in a directory in the user's checkout path to work. __It must parse 
> as a valid Python file__, or you'll see an error when using rbt.


Diffs
-

  support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 

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


Testing
---

Modified the commit message with `git rebase` and ran 
`./support/post-reviews.py` to observe the above error messages.
Used the valid commit message for this patch and created it by running 
`./support/post-reviews.py`.


Thanks,

Michael Park

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Joseph Wu]

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

(Updated July 9, 2015, 9:49 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad.


Changes
---

Re-word comment over "finalize".  Add periods at the end of some @param 
declarations.


Repository: mesos


Description
---

Doxygen-ification of comments in libprocess process headers.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
59b50af86e059463a01f3c83701bc5fd143d51a4 

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


Testing
---

`doxygen ../Doxyfile` and visually verified the HTML output.


Thanks,

Joseph Wu

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Joseph Wu]

> On July 9, 2015, 5:49 a.m., Joerg Schad wrote:
> > 3rdparty/libprocess/include/process/process.hpp, line 40
> > 
> >
> > /s/**Note**:/**NOTE:**

The "NOTE" is already in all caps, as per the Markdown style guide:
https://github.com/apache/mesos/blob/master/docs/mesos-markdown-style-guide.md#notesemphasis


- Joseph


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


On July 7, 2015, 5:24 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36273/
> ---
> 
> (Updated July 7, 2015, 5:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen-ification of comments in libprocess process headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 59b50af86e059463a01f3c83701bc5fd143d51a4 
> 
> Diff: https://reviews.apache.org/r/36273/diff/
> 
> 
> Testing
> ---
> 
> `doxygen ../Doxyfile` and visually verified the HTML output.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35998]

All tests passed.

- Mesos ReviewBot


On July 9, 2015, 4:02 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35998/
> ---
> 
> (Updated July 9, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 86c12a5 
> 
> Diff: https://reviews.apache.org/r/35998/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 35687: Added capabilities to state.json

[Vinod Kone]

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



src/master/http.cpp (lines 124 - 125)


were you not able to use 'foreach' here?

foreach(FrameworkInfo::Capability& capability, 
framework.info.capabilities()) {

}



src/tests/master_tests.cpp (lines 2994 - 2996)


pull this below #2998 so that webui_url testing is all in one block.

also, you captured 'Capability' into an JSON::array but doing nothing with 
it? did you forget to add an EXPECT_EQ to make sure this value is the same as 
the one you added in #2958?


- Vinod Kone


On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> ---
> 
> (Updated July 9, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Marco Massenzio and Vinod Kone.
> 
> 
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added capabilities to state.json and test for the same
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 
>   src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22 
> 
> Diff: https://reviews.apache.org/r/35687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.

[Alexander Rukletsov]

> On July 8, 2015, 5:59 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 133-135
> > 
> >
> > And we introduce a libprocess dependency into `Allocator` interface. I 
> > think it's a prominent step, right now an allocator implementation is not 
> > even required to be asynchronous. I don't want to say it's a bad change, I 
> > want us to think a bit more about the consequences of such step. Maybe it 
> > is worth discussing on the dev list.
> 
> Jie Yu wrote:
> Most of our module interfaces depends on libprocess (e.g., resource 
> estimator, qos controller, isolator, etc.). Why do you think this is a 
> prominent step?
> 
> > right now an allocator implementation is not even required to be 
> asynchronous
> 
> I think at the time we introduced modules, we agreed on that internal 
> interfaces are subject to change without needing a formal deprecation cycle. 
> It's up to the module developer to use proper versioning to deal with changes 
> in the interfaces.

To clarify, I think it's fine updating an interface, but giving the growing 
complexity of allocator, introducing a libprocess dependency doesn't make it 
easier : ). However, if we all share same concerns and are eager to refactor 
the allocator interface in the near future, I'm fine with the change.


- Alexander


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


On June 26, 2015, 10:55 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35947/
> ---
> 
> (Updated June 26, 2015, 10:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, 
> and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, 
> `/create` and `/destroy`.
> 
> This API is similar to `updateSlave` which is currently very specific to 
> oversubscription.
> I considered consolidating `updateAvailable` and `updateSlave` but it will 
> require making offers be generated within the allocator to enable this.
> 
> In specific, `updateAvailable` could fail if there aren't sufficient 
> available resources to update, whereas `updateSlave` avoids failing by 
> keeping the allocator in an over-allocated state. For `updateSlave`, leaving 
> the allocator in an over-allocated state is ok. This is because it does not 
> modify resources therefore `total - allocated` will work out to __empty__. 
> `updateAvailable` cannot leave the allocator in an over-allocated state, 
> because it modifies resources, and therefore `total - allocated` is not 
> guaranteed to yield __empty__.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 
>   src/master/allocator/mesos/allocator.hpp 
> 72470ec7f56f84a9a9815c09adb88def90ef672f 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3264d145d52b48852878abf7ab9be29ab98208cc 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3258840135290cd008ca09235d18b7f093dafd2e 
>   src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 
> 
> Diff: https://reviews.apache.org/r/35947/diff/
> 
> 
> Testing
> ---
> 
> (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and 
> `HierarchicalAllocatorTest.updateAvailableFail`
> (2) `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Till Toenshoff]

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

Ship it!



support/post-reviews.py (lines 106 - 119)


Nice addition.


- Till Toenshoff


On July 8, 2015, 9:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35777/
> ---
> 
> (Updated July 8, 2015, 9:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handles the case where the ReviewBoard URL is invalid.
> 
> ```
> af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
> (29 seconds ago)
> 
> Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
> ```
> 
> Reads `REVIEWBOARD_URL` from `.reviewboardrc` if it's available. We use 
> `imp.load_source` to import the `.reviewboardrc` file.
> > __.reviewboardrc__
> >
> > The .reviewboardrc file is a generic place for configuring a repository. 
> > This must be in a directory in the user’s checkout path to work. __It must 
> > parse as a valid Python file__, or you’ll see an error when using rbt.
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 
> 
> Diff: https://reviews.apache.org/r/35777/diff/
> 
> 
> Testing
> ---
> 
> Modified the commit message with `git rebase` and ran 
> `./support/post-reviews.py` to observe the above error messages.
> Used the valid commit message for this patch and created it by running 
> `./support/post-reviews.py`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.

[Till Toenshoff]

> On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48
> > 
> >
> > It would be good to explain why we are opting this way. How is this 
> > useful? OK, maybe nobody knows and we don't want to touch the code? There 
> > are  various other inexplicable outcomes here.
> 
> Till Toenshoff wrote:
> While these may seem inexplicable, they are defined for the standard 
> implementation of this function. I guess what you would like to see is 
> something more along the lines of a `dirname()` and `filename()` to produce 
> results that are more straightforward.
> 
> I think adding those as an additional implementation is valuable and Cody 
> expressed that as well in the past.
> 
> Bernd Mathiske wrote:
> A TODO then?

Aye, adding that to Cody's existing TODO.


- Till


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


On June 29, 2015, 11:43 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35998/
> ---
> 
> (Updated June 29, 2015, 11:43 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
> 
> Diff: https://reviews.apache.org/r/35998/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 35687: Added capabilities to state.json

[Aditi Dixit]

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

(Updated July 9, 2015, 4:04 p.m.)


Review request for mesos, Marco Massenzio and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Added capabilities to state.json and test for the same


Diffs (updated)
-

  src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 
  src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22 

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


Testing
---

make check


Thanks,

Aditi Dixit

Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.

[Till Toenshoff]

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

(Updated July 9, 2015, 4:02 p.m.)


Review request for mesos, Bernd Mathiske and Joerg Schad.


Changes
---

Addressed comments.


Repository: mesos


Description
---

see summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 86c12a5 

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


Testing
---


Thanks,

Till Toenshoff

Re: Review Request 36336: Expose major, minor and patch components from stout Version

[Paul Brett]

> On July 9, 2015, 2:46 a.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 36
> > 
> >
> > Why do we want to rename major/minor/patch to 
> > primary/secondary/tertiary? The former is a widely accepted format.

g++ defines _GNU_SOURCE (required for libstdc++).  This #includes sysmacros.h 
which #define major and minor as macros for makedev.


- Paul


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


On July 8, 2015, 10:50 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36336/
> ---
> 
> (Updated July 8, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, 
> Kapil Arya, and Vinod Kone.
> 
> 
> Bugs: MESOS-3020
> https://issues.apache.org/jira/browse/MESOS-3020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose major, minor and patch components from stout Version
> 
> Note: The names major and minor are not available when compiling with GCC 
> because it pulls in the sysmacros.h header implicitly which define these for 
> makedev.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 8692323d28131cd5706dde0503d49f8f0b0a1aeb 
> 
> Diff: https://reviews.apache.org/r/36336/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Till Toenshoff]

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

Ship it!



3rdparty/libprocess/src/tests/process_tests.cpp (line 1680)


A short description on the target of this test would be terriffic -- will 
fix this while committing.


- Till Toenshoff


On June 17, 2015, 3:42 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 17, 2015, 3:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When serving a static file, libprocess returns the header `Last-Modified` 
> which is used by browsers to control Cache.
> When a http request arrives containing the header `If-Modified-Since`, a 
> response `304 Not Modified` is returned if the date in the request and the 
> modification time (as returned by doing `stat` in the file) coincide.
> Unit tests added.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> e47cc7afbc8110759edf25a2dc05d09eda25c417 
>   3rdparty/libprocess/src/process.cpp 
> a67a3afdb30d23eb1b265b04ae662f64e874b6c6 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 34703: Added stream manipulators for the Time object.

[Till Toenshoff]

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

Ship it!


Will fix the below issue while comitting.


3rdparty/libprocess/src/time.cpp (line 1)


This misses the “Apache License Version 2.0” header - see our style-guide 
on "File Headers".

```
/**  
 * Licensed under the Apache License, Version 2.0 (the "License");  
 * you may not use this file except in compliance with the License.  
 * You may obtain a copy of the License at  
 *
 * http://www.apache.org/licenses/LICENSE-2.0  
 *
 * Unless required by applicable law or agreed to in writing, software  
 * distributed under the License is distributed on an "AS IS" BASIS,  
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. 
 
 * See the License for the specific language governing permissions and  
 * limitations under the License  
 */
```


- Till Toenshoff


On June 17, 2015, 3:42 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34703/
> ---
> 
> (Updated June 17, 2015, 3:42 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and 
> Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds some manipulator classes which allows formatting Time objects to 
> ostreams.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 
>   3rdparty/libprocess/include/process/time.hpp 
> c5ab2a3cfa83590eb6612152ae365dd67f51cea9 
>   3rdparty/libprocess/src/tests/time_tests.cpp 
> be314182c65c05d439b81aa5248a71d93f6f0a0b 
>   3rdparty/libprocess/src/time.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34703/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36314: Added test for passing total slave's resources in ResourceUsage.

[Bartek Plotka]

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

(Updated July 9, 2015, 1:09 p.m.)


Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod 
Kone.


Changes
---

Attaching this request to resolved Mesos issue is not a good idea (:


Repository: mesos


Description
---

Follow up tests for https://reviews.apache.org/r/36204/


Diffs
-

  src/tests/slave_tests.cpp ea0ddf38444915a1cda71cce6a8897803fa49198 

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


Testing
---

make check.


Thanks,

Bartek Plotka

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Joerg Schad]

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

Ship it!


After fixing the above comments...

- Joerg Schad


On July 8, 2015, 12:24 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36273/
> ---
> 
> (Updated July 8, 2015, 12:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen-ification of comments in libprocess process headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 59b50af86e059463a01f3c83701bc5fd143d51a4 
> 
> Diff: https://reviews.apache.org/r/36273/diff/
> 
> 
> Testing
> ---
> 
> `doxygen ../Doxyfile` and visually verified the HTML output.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Joerg Schad]

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



3rdparty/libprocess/include/process/process.hpp (line 40)


/s/**Note**:/**NOTE:**



3rdparty/libprocess/include/process/process.hpp (line 83)


That seems very long for the brief description... Feel free to drop 
though...



3rdparty/libprocess/include/process/process.hpp (line 407)


see above



3rdparty/libprocess/include/process/process.hpp (line 423)


the comma seems off here...



3rdparty/libprocess/include/process/process.hpp (line 425)


I know you didn't touch this directly, but as you touched the comment block 
anyhow could you add a perdiod at the end?


- Joerg Schad


On July 8, 2015, 12:24 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36273/
> ---
> 
> (Updated July 8, 2015, 12:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen-ification of comments in libprocess process headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 59b50af86e059463a01f3c83701bc5fd143d51a4 
> 
> Diff: https://reviews.apache.org/r/36273/diff/
> 
> 
> Testing
> ---
> 
> `doxygen ../Doxyfile` and visually verified the HTML output.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.

[Bernd Mathiske]

> On July 7, 2015, 2:50 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48
> > 
> >
> > It would be good to explain why we are opting this way. How is this 
> > useful? OK, maybe nobody knows and we don't want to touch the code? There 
> > are  various other inexplicable outcomes here.
> 
> Till Toenshoff wrote:
> While these may seem inexplicable, they are defined for the standard 
> implementation of this function. I guess what you would like to see is 
> something more along the lines of a `dirname()` and `filename()` to produce 
> results that are more straightforward.
> 
> I think adding those as an additional implementation is valuable and Cody 
> expressed that as well in the past.

A TODO then?


- Bernd


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


On June 29, 2015, 4:43 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35998/
> ---
> 
> (Updated June 29, 2015, 4:43 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
> 
> Diff: https://reviews.apache.org/r/35998/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.

[Till Toenshoff]

> On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 24
> > 
> >
> > I know that in many other places we have written "Basic abstraction", 
> > but this really adds no meaning. So let's stop that! 
> > 
> > And we don't deal with just about any file system here (e.g. FAT32), do 
> > we?
> > 
> > Suggestion:
> > 
> > "Represents UNIX file systems paths and offers common path 
> > manipulations."
> > 
> > Or something like that :-)
> 
> Till Toenshoff wrote:
> Yep. Good point, let update that as well.

s/let/let us/


- Till


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


On June 29, 2015, 11:43 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35998/
> ---
> 
> (Updated June 29, 2015, 11:43 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
> 
> Diff: https://reviews.apache.org/r/35998/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.

[Till Toenshoff]

> On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 39
> > 
> >
> > Do you mean std::basename()?

Nope, the free C function `basename()` is part of the POSIX pattern matching 
implementations and as such not part of the `std` namespace (not even wrapped). 
We commonly mark such function using the explicit, global namespace `::` in our 
C++ codebase.


> On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 24
> > 
> >
> > I know that in many other places we have written "Basic abstraction", 
> > but this really adds no meaning. So let's stop that! 
> > 
> > And we don't deal with just about any file system here (e.g. FAT32), do 
> > we?
> > 
> > Suggestion:
> > 
> > "Represents UNIX file systems paths and offers common path 
> > manipulations."
> > 
> > Or something like that :-)

Yep. Good point, let update that as well.


> On July 7, 2015, 9:50 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48
> > 
> >
> > It would be good to explain why we are opting this way. How is this 
> > useful? OK, maybe nobody knows and we don't want to touch the code? There 
> > are  various other inexplicable outcomes here.

While these may seem inexplicable, they are defined for the standard 
implementation of this function. I guess what you would like to see is 
something more along the lines of a `dirname()` and `filename()` to produce 
results that are more straightforward.

I think adding those as an additional implementation is valuable and Cody 
expressed that as well in the past.


- Till


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


On June 29, 2015, 11:43 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35998/
> ---
> 
> (Updated June 29, 2015, 11:43 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
> 
> Diff: https://reviews.apache.org/r/35998/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.

[Till Toenshoff]

> On July 1, 2015, 7:05 a.m., Joerg Schad wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 44
> > 
> >
> > Did you check how this displays in the rendered version?

Good point, looks horrible - using a markdown-styled table fixes it.


- Till


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


On June 29, 2015, 11:43 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35998/
> ---
> 
> (Updated June 29, 2015, 11:43 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> a4afdad0b5f053186ace4d6a37b41cd02e7d415b 
> 
> Diff: https://reviews.apache.org/r/35998/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 36193: Improved Doxygen-Styleguide.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On July 6, 2015, 2:01 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36193/
> ---
> 
> (Updated July 6, 2015, 2:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved Doxygen-Styleguide for clarifying discussions arising from 
> https://reviews.apache.org/r/36141/.
> 
> 
> Diffs
> -
> 
>   docs/mesos-doxygen-style-guide.md 72156c72fc40f72740e57d47d0436c60f6d05bd7 
> 
> Diff: https://reviews.apache.org/r/36193/diff/
> 
> 
> Testing
> ---
> 
> Checked rendered markdown.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36216: Only run netcat tests when nc is available.

[Joerg Schad]

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


Isn't this submitted already?

- Joerg Schad


On July 6, 2015, 10:58 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36216/
> ---
> 
> (Updated July 6, 2015, 10:58 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Chi Zhang, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> nc is not available on CentOS 7.1 and all the tests using nc will fail.
> Added a new NetcatFilter so we detect nc before running these tests.
> 
> 
> Diffs
> -
> 
>   src/tests/docker_containerizer_tests.cpp 
> d4ccb0b30fe27980846d913e292d2e18fd3d1055 
>   src/tests/environment.cpp 3726e5d95b9388f0881184e1b4874e1eb2e7f531 
>   src/tests/port_mapping_tests.cpp ac49cdfdcf6baf00ac2907e193c683c8b6c83ffb 
> 
> Diff: https://reviews.apache.org/r/36216/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.

[Till Toenshoff]

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



src/tests/hierarchical_allocator_tests.cpp (line 785)


Would be great if you could add a brief description as a comment to these 
tests as done for most others here as well.



src/tests/hierarchical_allocator_tests.cpp (line 828)


see above.


- Till Toenshoff


On June 26, 2015, 10:55 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35947/
> ---
> 
> (Updated June 26, 2015, 10:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, 
> and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, 
> `/create` and `/destroy`.
> 
> This API is similar to `updateSlave` which is currently very specific to 
> oversubscription.
> I considered consolidating `updateAvailable` and `updateSlave` but it will 
> require making offers be generated within the allocator to enable this.
> 
> In specific, `updateAvailable` could fail if there aren't sufficient 
> available resources to update, whereas `updateSlave` avoids failing by 
> keeping the allocator in an over-allocated state. For `updateSlave`, leaving 
> the allocator in an over-allocated state is ok. This is because it does not 
> modify resources therefore `total - allocated` will work out to __empty__. 
> `updateAvailable` cannot leave the allocator in an over-allocated state, 
> because it modifies resources, and therefore `total - allocated` is not 
> guaranteed to yield __empty__.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 
>   src/master/allocator/mesos/allocator.hpp 
> 72470ec7f56f84a9a9815c09adb88def90ef672f 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3264d145d52b48852878abf7ab9be29ab98208cc 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3258840135290cd008ca09235d18b7f093dafd2e 
>   src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 
> 
> Diff: https://reviews.apache.org/r/35947/diff/
> 
> 
> Testing
> ---
> 
> (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and 
> `HierarchicalAllocatorTest.updateAvailableFail`
> (2) `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36023: Improved the documentation of Containerizer::launch() to clarify the failure cases.

[Till Toenshoff]

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

Ship it!


This is really helpful as it has become a little bit of a confusion milestone 
for many.

- Till Toenshoff


On June 29, 2015, 9:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36023/
> ---
> 
> (Updated June 29, 2015, 9:29 p.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved the documentation of Containerizer::launch() to clarify the failure 
> cases.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 
> 
> Diff: https://reviews.apache.org/r/36023/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 23784: Missing Apache headers for stout

[Till Toenshoff]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/configure.ac (lines 13 - 14)


These actually don't add any value, I think. We should get rid of them at 
some point (not part of this RR!).


- Till Toenshoff


On July 8, 2015, 8:51 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23784/
> ---
> 
> (Updated July 8, 2015, 8:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Dominic Hamon.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding missing Apache licence headers on .cpp, .hpp and Makefiles
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 9df5de4 
>   3rdparty/libprocess/3rdparty/stout/configure.ac 1988d19 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am ff6fb28 
>   3rdparty/libprocess/3rdparty/stout/tests/bits_tests.cpp c47bd13 
>   3rdparty/libprocess/3rdparty/stout/tests/bytes_tests.cpp 1fe7e08 
>   3rdparty/libprocess/3rdparty/stout/tests/cache_tests.cpp f8d6e42 
>   3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 724c5fe 
>   3rdparty/libprocess/3rdparty/stout/tests/error_tests.cpp 35a62b1 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 2a6f67b 
>   3rdparty/libprocess/3rdparty/stout/tests/gzip_tests.cpp 2211f31 
>   3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp 4a8176b 
>   3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp 97a7167 
>   3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp 1fa8fc1 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp c50480b 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 0011f08 
>   3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp 0644d99 
>   3rdparty/libprocess/3rdparty/stout/tests/mac_tests.cpp b80fff9 
>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp f5f20ee 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 11f3bf7 
>   3rdparty/libprocess/3rdparty/stout/tests/none_tests.cpp 1c1f8be 
>   3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp 4a0f60b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp fee942c 
>   3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 6d2d3d5 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp d7d4552 
>   3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp dd3700d 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp b3ce131 
>   3rdparty/libprocess/3rdparty/stout/tests/set_tests.cpp b3cd5f8 
>   3rdparty/libprocess/3rdparty/stout/tests/some_tests.cpp ed48279 
>   3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 9733b2e 
>   3rdparty/libprocess/3rdparty/stout/tests/thread_tests.cpp 6871e8b 
>   3rdparty/libprocess/3rdparty/stout/tests/uuid_tests.cpp 7ac8fc0 
> 
> Diff: https://reviews.apache.org/r/23784/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 23783: Missing Apache headers for libprocess

[Till Toenshoff]

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

Ship it!


Looks great, thanks for solving this tedious task.

- Till Toenshoff


On July 8, 2015, 7:47 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23783/
> ---
> 
> (Updated July 8, 2015, 7:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Dominic Hamon.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding missing Apache licence headers on .cpp, .hpp and Makefiles
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 358893c 
>   3rdparty/libprocess/configure.ac c197baf 
>   3rdparty/libprocess/examples/example.cpp 3fb4ef5 
>   3rdparty/libprocess/include/Makefile.am d01880c 
>   3rdparty/libprocess/include/process/address.hpp 88946d5 
>   3rdparty/libprocess/include/process/async.hpp 9af3cc0 
>   3rdparty/libprocess/include/process/clock.hpp 8dc7be8 
>   3rdparty/libprocess/include/process/collect.hpp c713c1b 
>   3rdparty/libprocess/include/process/defer.hpp 7c04736 
>   3rdparty/libprocess/include/process/deferred.hpp 3746d69 
>   3rdparty/libprocess/include/process/delay.hpp 29e3532 
>   3rdparty/libprocess/include/process/dispatch.hpp 617fd43 
>   3rdparty/libprocess/include/process/event.hpp ad4a8f4 
>   3rdparty/libprocess/include/process/executor.hpp 157a1d2 
>   3rdparty/libprocess/include/process/filter.hpp aa0c91b 
>   3rdparty/libprocess/include/process/future.hpp a9e765d 
>   3rdparty/libprocess/include/process/gc.hpp e83c636 
>   3rdparty/libprocess/include/process/gmock.hpp 0fd3f8c 
>   3rdparty/libprocess/include/process/gtest.hpp 8518c38 
>   3rdparty/libprocess/include/process/help.hpp 07e99f1 
>   3rdparty/libprocess/include/process/http.hpp 8d9adc5 
>   3rdparty/libprocess/include/process/id.hpp e586937 
>   3rdparty/libprocess/include/process/io.hpp 6388770 
>   3rdparty/libprocess/include/process/latch.hpp 9d010f0 
>   3rdparty/libprocess/include/process/limiter.hpp 444aa1b 
>   3rdparty/libprocess/include/process/logging.hpp 80b1e8f 
>   3rdparty/libprocess/include/process/message.hpp c67c5e1 
>   3rdparty/libprocess/include/process/metrics/counter.hpp f9cab39 
>   3rdparty/libprocess/include/process/metrics/gauge.hpp 7d02cd5 
>   3rdparty/libprocess/include/process/metrics/metric.hpp 616f060 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp aa44992 
>   3rdparty/libprocess/include/process/metrics/timer.hpp 813115a 
>   3rdparty/libprocess/include/process/mime.hpp 0abeac1 
>   3rdparty/libprocess/include/process/mutex.hpp 37ea49a 
>   3rdparty/libprocess/include/process/network.hpp f4192e0 
>   3rdparty/libprocess/include/process/once.hpp 2b81df3 
>   3rdparty/libprocess/include/process/owned.hpp 0541113 
>   3rdparty/libprocess/include/process/pid.hpp e0a9fce 
>   3rdparty/libprocess/include/process/process.hpp 59b50af 
>   3rdparty/libprocess/include/process/profiler.hpp 86050b1 
>   3rdparty/libprocess/include/process/protobuf.hpp 91493de 
>   3rdparty/libprocess/include/process/queue.hpp 5be29db 
>   3rdparty/libprocess/include/process/reap.hpp 5e5051a 
>   3rdparty/libprocess/include/process/run.hpp a0d7286 
>   3rdparty/libprocess/include/process/sequence.hpp d755b34 
>   3rdparty/libprocess/include/process/shared.hpp d80fb7f 
>   3rdparty/libprocess/include/process/socket.hpp 4d95183 
>   3rdparty/libprocess/include/process/statistics.hpp 929fb8d 
>   3rdparty/libprocess/include/process/subprocess.hpp 869e3d9 
>   3rdparty/libprocess/include/process/system.hpp 4fb3c83 
>   3rdparty/libprocess/include/process/time.hpp c5ab2a3 
>   3rdparty/libprocess/include/process/timeout.hpp 5c46d70 
>   3rdparty/libprocess/include/process/timer.hpp e5d71f6 
>   3rdparty/libprocess/include/process/timeseries.hpp ec0ac67 
>   3rdparty/libprocess/src/clock.cpp dd726c1 
>   3rdparty/libprocess/src/config.hpp d5084bf 
>   3rdparty/libprocess/src/decoder.hpp 85ce9e3 
>   3rdparty/libprocess/src/encoder.hpp b898658 
>   3rdparty/libprocess/src/event_loop.hpp af57fe2 
>   3rdparty/libprocess/src/fatal.hpp 34314fd 
>   3rdparty/libprocess/src/fatal.cpp b2934e0 
>   3rdparty/libprocess/src/gate.hpp e5c9379 
>   3rdparty/libprocess/src/http.cpp 0898335 
>   3rdparty/libprocess/src/io.cpp 4944e28 
>   3rdparty/libprocess/src/latch.cpp cba4dcd 
>   3rdparty/libprocess/src/libev.hpp a0a2f49 
>   3rdparty/libprocess/src/libev.cpp 2b8c68d 
>   3rdparty/libprocess/src/libev_poll.cpp 6191be3 
>   3rdparty/libprocess/src/libevent.hpp 47b93f1 
>   3rdparty/libprocess/src/libevent.cpp 67e7501 
>   3rdparty/libprocess/src/libevent_poll.cpp d0b8946 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 11c1b70 
>   3rdparty/libprocess/src/libevent_

Re: Review Request 36218: Doxygen styleguide revisions based on conversation from https://reviews.apache.org/r/36193/.

[Joerg Schad]

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

Ship it!


Ship It!

- Joerg Schad


On July 8, 2015, 8:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36218/
> ---
> 
> (Updated July 8, 2015, 8:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen styleguide revisions based on conversation from 
> https://reviews.apache.org/r/36193/.
> 
> 
> Diffs
> -
> 
>   docs/mesos-doxygen-style-guide.md 72156c72fc40f72740e57d47d0436c60f6d05bd7 
> 
> Diff: https://reviews.apache.org/r/36218/diff/
> 
> 
> Testing
> ---
> 
> Checked rendered markdown.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36041: The "configure" phase breaks with the IBM JVM.

[Till Toenshoff]

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


Do we have a JIRA issue that would cover this issue? If so, please add it to 
this review-request within the "Bugs" section. If there is no JIRA, please 
create one that describes the problem briefly.

Next question; once the configuration phase is patched like this, do the tests 
then fully work using the IBM JVM? In other words, right now I don't know if 
this patch is sufficient to fully support the IBM JVM.

- Till Toenshoff


On June 30, 2015, 9:09 a.m., Jihun Kang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36041/
> ---
> 
> (Updated June 30, 2015, 9:09 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The "configure" phase breaks with the IBM JVM.
> 
> 
> Diffs
> -
> 
>   configure.ac 92909580ea735a1ccffbe03a2eb44e3d07566488 
> 
> Diff: https://reviews.apache.org/r/36041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jihun Kang
> 
>