Re: Review Request 39645: Updates /site to reflect Niklas' 0.25.0 changes that weren't reflected in git.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39645]

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

Error:
 2015-10-26 08:56:45 URL:https://reviews.apache.org/r/39645/diff/raw/ 
[6490/6490] -> "39645.patch" [1]
39645.patch:43: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Successfully applied: Updates /site to reflect Niklas' 0.25.0 changes that 
weren't reflected in git.

Updates /site to reflect Niklas' 0.25.0 changes that weren't reflected in git.


Review: https://reviews.apache.org/r/39645
site/source/blog/2015-10-12-mesos-0-25-0-released.md:37: new blank line at EOF.
Failed to commit patch

- Mesos ReviewBot


On Oct. 26, 2015, 8:46 a.m., Dave Lester wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39645/
> ---
> 
> (Updated Oct. 26, 2015, 8:46 a.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updates /site to reflect Niklas' 0.25.0 changes that weren't reflected in git.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2015-10-12-mesos-0-25-0-released.md PRE-CREATION 
>   site/source/downloads.html.md 49827fff0dedebc4738afbd02f17d746de72b5d6 
>   site/source/index.html.md b43325ab14c79f6b27db49fd52ea14d8d77c46c6 
> 
> Diff: https://reviews.apache.org/r/39645/diff/
> 
> 
> Testing
> ---
> 
> The website was built locally using rake, and these changes were reflected 
> properly.
> 
> 
> Thanks,
> 
> Dave Lester
> 
>

Review Request 39645: Updates /site to reflect Niklas' 0.25.0 changes that weren't reflected in git.

[Dave Lester]

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

Review request for mesos, Adam B and Niklas Nielsen.


Repository: mesos


Description
---

Updates /site to reflect Niklas' 0.25.0 changes that weren't reflected in git.


Diffs
-

  site/source/blog/2015-10-12-mesos-0-25-0-released.md PRE-CREATION 
  site/source/downloads.html.md 49827fff0dedebc4738afbd02f17d746de72b5d6 
  site/source/index.html.md b43325ab14c79f6b27db49fd52ea14d8d77c46c6 

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


Testing
---

The website was built locally using rake, and these changes were reflected 
properly.


Thanks,

Dave Lester

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Qian Zhang]

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



src/master/allocator/mesos/hierarchical.cpp (line 996)


These newly added code makes allocate() a huge method (more than 200 
lines), maybe move these codes into a separate method?



src/master/allocator/mesos/hierarchical.cpp (lines 1017 - 1020)


Why do we put these code inside the framework sorters foreach loop? I do 
not see it is related to framework.
If we really want to put these code here, then I think we also need to 
recalculate roleAllocatedResources every time when we allocate some resources 
to a framework of the role, and once the quota for the role is satifised, break.



src/master/allocator/mesos/hierarchical.cpp (lines 1055 - 1057)


Since we know the guarentee of each quota'ed role and the gap between it 
and role's current allocation, I think it might be better to do the 
"find-grained" allocation, i.e., only allocate resources to fill the gap. 
Otherwise, we may run into the situation that we allocate too much resource to 
satisfy a role's guarentee but another role's guarentee can not be satisfied.



src/master/allocator/mesos/hierarchical.cpp (line 1058)


We also have this code ```offerable[frameworkId][slaveId] = resources;``` 
in the following DRF allocation stage, so that means for a framework, the 
resources we allocate to it here will be overwritten by the the resources we 
allocate to it in DRF allocation stage? This seems not correct, maybe change 
the code in DRF allocation stage from "=" to "+="?



src/master/allocator/mesos/hierarchical.cpp (line 1097)


If we *continue* from here, that means the following DRF allocation stage 
will be skipped? I think that is not what we want. So I guess it should be:
```
if (allocatable(resources)) {
  // Lay aside the resources.
  laidAsideResources[slaveId] = resources;
  slaves[slaveId].allocated += resources;
}
```


- Qian Zhang


On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 24, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39620: Windows: Included headers to make `stout/windows.hpp` standalone.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Oct. 24, 2015, 9:32 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39620/
> ---
> 
> (Updated Oct. 24, 2015, 9:32 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Included headers to make `stout/windows.hpp` standalone.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 7a8819541506f57f70a9b577dc97a76fc26ebaa8 
> 
> Diff: https://reviews.apache.org/r/39620/diff/
> 
> 
> Testing
> ---
> 
> `make check` from CMake on OS X 10.10.
> `make check` from Autotools on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> PLEASE NOTE: I am on a terrible network with proxy problems and I can't SSH 
> into my Ubuntu box to test this from Ubuntu
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39533: SSLTest refactor: Change test #ifdefs so this class exists with or without --enable-ssl.

[Joseph Wu]

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

(Updated Oct. 26, 2015, 3:36 p.m.)


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


Changes
---

Tweak ifdef-ing to be more readable.


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


Repository: mesos


Description
---

When `--enable-ssl` is not present, the body of the `SSLTemporaryDirectoryTest` 
is #ifdef'd out,
such that `SSLTemporaryDirectoryTest` is an empty wrapper around 
`TemporaryDirectoryTest`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
91991f509e480a94de00c6bf20ff0abf083dda8a 

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


Testing
---

Testing done in the next review.


Thanks,

Joseph Wu

Re: Review Request 39628: Clear the suppressed flag when deactive a framework

[Vinod Kone]

> On Oct. 25, 2015, 2:51 p.m., Ben Mahler wrote:
> > Can you please add a test that would have caught this issue?
> 
> Guangya Liu wrote:
> I think this is a bug, I tested without my code change, the test also 
> failed sometimes. Shall we file a bug for this?

the failed test looks unrelated. please do file a bug. also, please write a 
test for this change.


- Vinod


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


On Oct. 25, 2015, 2:38 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39628/
> ---
> 
> (Updated Oct. 25, 2015, 2:38 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3802
> https://issues.apache.org/jira/browse/MESOS-3802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clear the suppressed flag when deactive a framework
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39628/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39604: Added function that verifies prerequisites for using Linux launcher.

[Jie Yu]

> On Oct. 26, 2015, 6:55 p.m., Jie Yu wrote:
> > src/slave/containerizer/linux_launcher.hpp, line 39
> > 
> >
> > Cna you rename it to 'available'? We tend to omit 'is' or 'are' for 
> > those functions (e.g., cgroups::enabled(), not cgroups::areEnabled()).

I fixed it for you. The patch is committed.


- Jie


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


On Oct. 23, 2015, 11:48 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39604/
> ---
> 
> (Updated Oct. 23, 2015, 11:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Joris Van Remoortere, and Kapil 
> Arya.
> 
> 
> Bugs: MESOS-3800
> https://issues.apache.org/jira/browse/MESOS-3800
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We recently switched to using LinuxLauncher by default 
> (https://github.com/apache/mesos/blame/master/src/slave/containerizer/mesos/containerizer.cpp#L217-L234).
>  This causes problems with running the slave in a Docker container. 
> 
> This patch introduces a function for checking whether the machine is suited 
> for running Linux launcher. For the time being it checks whether the freezer 
> cgroup subsystem enabled, as the Linux launcher evolves we might want to add 
> more checks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/linux_launcher.hpp 
> 35b3315498d690ed66616617aa7d51455371fb5b 
>   src/slave/containerizer/linux_launcher.cpp 
> c03b89eb0678825b03a052874d6262f377a39e13 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d1fc5a460e7313828014eea999cf4e63dde01921 
> 
> Diff: https://reviews.apache.org/r/39604/diff/
> 
> 
> Testing
> ---
> 
> - Ran Mesoss tests in a Docker container where cgroup was not available.
> - Ran the new Jenkins script (https://reviews.apache.org/r/37787/).
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 39502: [DO NOT COMMIT] Sync v1/mesos.proto with docker, QoS, and AppC changes.

[Joseph Wu]

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

(Updated Oct. 26, 2015, 2:27 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, 
Kapil Arya, Niklas Nielsen, and Timothy Chen.


Changes
---

Changing title, since not all changes have been confirmed yet.


Summary (updated)
-

[DO NOT COMMIT] Sync v1/mesos.proto with docker, QoS, and AppC changes.


Repository: mesos


Description
---

Added in reviews:

* https://reviews.apache.org/r/37196/
* https://reviews.apache.org/r/37308/ <- Confirmed as unintentional
* https://reviews.apache.org/r/38253/
* https://reviews.apache.org/r/38367/
** NetworkInfo moved up the review chain.
* 
https://github.com/apache/mesos/commit/140311f263a6ae54d3d211c9c91e4bf55d2eb0f1


Diffs
-

  include/mesos/v1/mesos.proto 8131778fe5c5f3a47ae9300a811e3d857a22da6a 

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


Testing
---

`make`

For now, this review is meant for checking which of these differences are 
intentional.


Thanks,

Joseph Wu

Re: Review Request 38705: Added support for applying a review chain (apply-reviews.py).

[Joseph Wu]

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


Note: considering the amount of changes since my last review, my "Ship It" is 
no longer valid.


support/apply-reviews.py (line 15)


This won't work on Windows, since it'll construct "URL"s with (evil) 
backslashes.
(And considering how often we use this script for the CMake/Windows review 
chains... :)

This would be better:
https://docs.python.org/2/library/urlparse.html#urlparse.urljoin



support/apply-reviews.py (line 38)


Nit: Extra space after the if.



support/apply-reviews.py (line 78)


Nit: Extra space after function name.


- Joseph Wu


On Oct. 22, 2015, 11:16 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> ---
> 
> (Updated Oct. 22, 2015, 11:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38705/diff/
> 
> 
> Testing
> ---
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 39534: SSLTest refactor: Change MesosTest to inherit from the SSL helper class.

[Joris Van Remoortere]

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



src/tests/mesos.hpp (line 100)


As per offline discussion, let's comment explaining why it is ok to do this 
regardless of ssl configuration.


- Joris Van Remoortere


On Oct. 22, 2015, 12:04 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39534/
> ---
> 
> (Updated Oct. 22, 2015, 12:04 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3762
> https://issues.apache.org/jira/browse/MESOS-3762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/39534/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure
> make check
> ```
> 
> ```
> ../configure --enable-ssl --enable-libevent
> make check
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39498: SSLTest refactor: Change SSLTest to inherit from TemporaryDirectoryTest.

[Joris Van Remoortere]

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

Ship it!



3rdparty/libprocess/include/process/ssl/gtest.hpp (line 136)


indentation
snake_case?



3rdparty/libprocess/include/process/ssl/gtest.hpp (line 143)


I prefer irrecoverably :-)



3rdparty/libprocess/include/process/ssl/gtest.hpp (line 232)


`so as not to abort` ?


- Joris Van Remoortere


On Oct. 22, 2015, 12:04 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39498/
> ---
> 
> (Updated Oct. 22, 2015, 12:04 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3762
> https://issues.apache.org/jira/browse/MESOS-3762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This simplifies some of the cleanup logic, since temporary directories are
> cleaned up by the super class, or by the OS.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 91991f509e480a94de00c6bf20ff0abf083dda8a 
> 
> Diff: https://reviews.apache.org/r/39498/diff/
> 
> 
> Testing
> ---
> 
> Tests run in next review.
> 
> Tests are slightly slower, by a few dozen ms per test, since SetUp happens on 
> a per-test, rather than per-test-class basis.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39499: SSLTest refactor: Update docker registry tests to reflect changed cleanup logic of SSLTest.

[Joris Van Remoortere]

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

Ship it!


- Joris Van Remoortere


On Oct. 22, 2015, 12:04 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39499/
> ---
> 
> (Updated Oct. 22, 2015, 12:04 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3762
> https://issues.apache.org/jira/browse/MESOS-3762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 01d3025f59d4a2714a856fe0f3a57192be023990 
> 
> Diff: https://reviews.apache.org/r/39499/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure --enable-ssl --enable-libevent
> make check
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39501: SSLTest refactor: Split SSLTest into helpers which can be integrated into MesosTest.

[Joris Van Remoortere]

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

Ship it!



3rdparty/libprocess/include/process/ssl/gtest.hpp (lines 264 - 265)


whitespace.


- Joris Van Remoortere


On Oct. 22, 2015, 12:04 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39501/
> ---
> 
> (Updated Oct. 22, 2015, 12:04 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3762
> https://issues.apache.org/jira/browse/MESOS-3762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Splits `SSLTest` into:
> 
> * `SSLTemporaryDirectoryTest`, which contains only helpers.
> * `SSLTest`, which has some `SetUp` logic, like the original `SSLTest`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 91991f509e480a94de00c6bf20ff0abf083dda8a 
> 
> Diff: https://reviews.apache.org/r/39501/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure --enable-ssl --enable-libevent
> make check
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39533: SSLTest refactor: Change test #ifdefs so this class exists with or without --enable-ssl.

[Joris Van Remoortere]

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

Ship it!



3rdparty/libprocess/include/process/ssl/gtest.hpp (line 59)


As per discussion offline, let's make this more readable by doing something 
like:
```
// Your general comment.
#ifndef USE_SSL_SOCKET
class SSLTemporaryDirectoryTest : public TemporaryDirectoryTest {}
#else
class SSLTemporaryDirectoryTest : public TemporaryDirectoryTest
{
  // SSL specific behaviour and helper functions.
}
#endif
```


- Joris Van Remoortere


On Oct. 22, 2015, 12:04 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39533/
> ---
> 
> (Updated Oct. 22, 2015, 12:04 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3762
> https://issues.apache.org/jira/browse/MESOS-3762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When `--enable-ssl` is not present, the body of the 
> `SSLTemporaryDirectoryTest` is #ifdef'd out,
> such that `SSLTemporaryDirectoryTest` is an empty wrapper around 
> `TemporaryDirectoryTest`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 91991f509e480a94de00c6bf20ff0abf083dda8a 
> 
> Diff: https://reviews.apache.org/r/39533/diff/
> 
> 
> Testing
> ---
> 
> Testing done in the next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39417: Add --egress_flow_classifier_parent flag

[Ian Downes]

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



docs/configuration.md (lines 1545 - 1551)


Can we not infer where to put the classifier from any exisiting classifiers 
and only require the user to specify its parent if they want particular 
behavior?



src/slave/containerizer/isolators/network/port_mapping.cpp (line 398)


s/Failed tokenize/Failed to tokenize/



src/slave/containerizer/isolators/network/port_mapping.cpp (lines 401 - 405)


Why not add a 0x prefix if it's not present so you can use numify?

if (!strings::startsWith(tokens[0], "0x")) {
...
}



src/slave/containerizer/isolators/network/port_mapping.cpp (lines 407 - 409)


This will get refactored as above, but it's more helpful to specifically 
state which token couldn't be parsed.



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1415)


kill this newline so the error checking is paired with the Try<>



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1419)


add a newline in here



src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1423 - 1424)


Can we verify this somehow? What's the failure mode?



src/slave/flags.cpp (lines 506 - 513)


Why is this description different from the docs?


- Ian Downes


On Oct. 17, 2015, 5:29 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39417/
> ---
> 
> (Updated Oct. 17, 2015, 5:29 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When --egress_unique_flow_per_container is enabled, we need to install a flow 
> classifier (fq_codel) qdisc on egress side. This flag specifies where to 
> install it in the hierarchy. By default, we install it at root. But, for 
> example, if you have already installed HTB qdisc at root, you may want this 
> to be installed in other place than root, specify an HTB class ID as its 
> parent here.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 
> 
> Diff: https://reviews.apache.org/r/39417/diff/
> 
> 
> Testing
> ---
> 
> Manual tests, with and without a pre-installed HTB qdisc and classes.
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/slave/validation.hpp, line 31
> > 
> >
> > why space here?

Added it there after a review comment.


> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/slave/validation.cpp, line 75
> > 
> >
> > also print status.source()

We don't have a stringify for this.


> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/tests/executor_http_api_tests.cpp, line 505
> > 
> >
> > make sure this pattern is consistent everywhere else in this file.

Inversed advise here, killed this and made sure above pattern is consistent 
everywhere.


- Isabel


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


On Oct. 27, 2015, 1:20 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Oct. 27, 2015, 1:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e6169a0 
>   src/Makefile.am e797dac 
>   src/slave/http.cpp 3f7f71b 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39584: Windows: Implemented `os::rmdir.hpp`.

[Joseph Wu]

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (lines 35 
- 40)


Use `strings::endsWith`.

And you might also want to use a ternary conditional:
`currentPath = path + (strings::endsWith("\") ? "" : "\");`



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 42)


`X:\blah*` isn't clear.  Did you mean `currentPath` + `*`?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 51)


Nit: Errors don't end with periods.  Ditto for the other error messages.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 134)


You seem to be missing an `if (recursive)` here...


- Joseph Wu


On Oct. 23, 2015, 9:30 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39584/
> ---
> 
> (Updated Oct. 23, 2015, 9:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Implemented `os::rmdir.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> ba2836a72ceee948cb43364e80ada9f132f33d04 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 7f70c9ea7d57634b5bfd40523ba37561ec92a09a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/rmdir.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> b6afe0e76366d0bc68d37097ced83a1e14828d84 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> 3e6f2aafd0f541f512025dfa683ab4178701f7c4 
> 
> Diff: https://reviews.apache.org/r/39584/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39499: SSLTest refactor: Update docker registry tests to reflect changed cleanup logic of SSLTest.

[Joseph Wu]

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

(Updated Oct. 26, 2015, 3:34 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
cf8aac383d6c6ec875b34af0460120ff521ce20e 

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


Testing
---

```
../configure --enable-ssl --enable-libevent
make check
```


Thanks,

Joseph Wu

Re: Review Request 39501: SSLTest refactor: Split SSLTest into helpers which can be integrated into MesosTest.

[Joseph Wu]

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

(Updated Oct. 26, 2015, 3:35 p.m.)


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


Changes
---

Add space!


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


Repository: mesos


Description
---

Splits `SSLTest` into:

* `SSLTemporaryDirectoryTest`, which contains only helpers.
* `SSLTest`, which has some `SetUp` logic, like the original `SSLTest`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
91991f509e480a94de00c6bf20ff0abf083dda8a 

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


Testing
---

```
../configure --enable-ssl --enable-libevent
make check
```


Thanks,

Joseph Wu

Re: Review Request 39498: SSLTest refactor: Change SSLTest to inherit from TemporaryDirectoryTest.

[Joseph Wu]

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

(Updated Oct. 26, 2015, 3:33 p.m.)


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


Changes
---

Address comments :)


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


Repository: mesos


Description
---

This simplifies some of the cleanup logic, since temporary directories are
cleaned up by the super class, or by the OS.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
91991f509e480a94de00c6bf20ff0abf083dda8a 

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


Testing
---

Tests run in next review.

Tests are slightly slower, by a few dozen ms per test, since SetUp happens on a 
per-test, rather than per-test-class basis.


Thanks,

Joseph Wu

Re: Review Request 39534: SSLTest refactor: Change MesosTest to inherit from the SSL helper class.

[Joseph Wu]

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

(Updated Oct. 26, 2015, 3:37 p.m.)


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


Changes
---

Add comment above change.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 

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


Testing
---

```
../configure
make check
```

```
../configure --enable-ssl --enable-libevent
make check
```


Thanks,

Joseph Wu

Re: Review Request 38705: Added support for applying a review chain (apply-reviews.py).

[Vinod Kone]

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

Ship it!


modulo joseph's comments.


support/apply-reviews.py (line 57)


log the id of the violating review?


- Vinod Kone


On Oct. 23, 2015, 6:16 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> ---
> 
> (Updated Oct. 23, 2015, 6:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38705/diff/
> 
> 
> Testing
> ---
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 38618: Changed executor HTTP API tests

[Isabel Jimenez]

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

(Updated Oct. 27, 2015, 12:09 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

rebase


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


Repository: mesos


Description
---

The SUBSCRIBE Call is supposed to have a 'subscribe' message. This change is 
needed so test can pass tests after call validation.


Diffs (updated)
-

  src/tests/executor_http_api_tests.cpp e429d84 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Oct. 27, 2015, 12:23 a.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


Changes
---

rebase


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt e6169a0 
  src/Makefile.am e797dac 
  src/slave/http.cpp 3f7f71b 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 
  src/tests/executor_http_api_tests.cpp e429d84 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38844: Added unit tests for Call validation in Agent

[Isabel Jimenez]

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

(Updated Oct. 27, 2015, 12:37 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

rebase


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


Repository: mesos


Description
---

Unit tests for Call validation in Agent.


Diffs (updated)
-

  src/tests/executor_http_api_tests.cpp e429d84 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Oct. 27, 2015, 1:20 a.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


Changes
---

review


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt e6169a0 
  src/Makefile.am e797dac 
  src/slave/http.cpp 3f7f71b 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 
  src/tests/executor_http_api_tests.cpp e429d84 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38844: Added unit tests for Call validation in Agent

[Vinod Kone]

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

Ship it!



src/tests/executor_http_api_tests.cpp (line 543)


s/responseSubscribe/response/

remove the suffices for response here and everywhere else below.



src/tests/executor_http_api_tests.cpp (line 562)


ditto.

s/responseUpdate/response/



src/tests/executor_http_api_tests.cpp (line 581)


ditto. just call it 'response'



src/tests/executor_http_api_tests.cpp (line 611)


s/valid//

s/executor_id/executor id/



src/tests/executor_http_api_tests.cpp (line 632)


ditto. just call it 'response'



src/tests/executor_http_api_tests.cpp (line 642)


s/valid//



src/tests/executor_http_api_tests.cpp (line 693)


ditto. just call it 'response'


- Vinod Kone


On Oct. 27, 2015, 12:37 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38844/
> ---
> 
> (Updated Oct. 27, 2015, 12:37 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unit tests for Call validation in Agent.
> 
> 
> Diffs
> -
> 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38844/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39497: Fixed quotation of interpolated variables in log messages.

[Joris Van Remoortere]

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

Ship it!


minus the removal of taskid quotations.

- Joris Van Remoortere


On Oct. 21, 2015, 2:37 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39497/
> ---
> 
> (Updated Oct. 21, 2015, 2:37 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3772
> https://issues.apache.org/jira/browse/MESOS-3772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Specifically, we should always quote executor IDs (because they are
> user-supplied and might contain whitespace), but we don't need to quote
> framework IDs. There's a bunch more work we could do here (e.g., escaping
> punctuation), but this is a start.
> 
> Also fix a few style issues with usage of "<<" operators.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp c99dc073f5ed5f635a41393ecb38ec13f21739d4 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/master.cpp 2cc814721a8c85b330a402b0ec54491a0b0db5aa 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef 
>   src/slave/slave.cpp 652697688fd9e9a6d064ef01fb032393412307b3 
>   src/slave/state.cpp 81c4b96d879f974f0dfba3fb977184122eab 
>   src/tests/containerizer.cpp 451c7be0bfd668ccd3325964cafcc4cbac941a94 
> 
> Diff: https://reviews.apache.org/r/39497/diff/
> 
> 
> Testing
> ---
> 
> make check, visual inspection.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.

[Joseph Wu]

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



support/apply-reviews.py (line 18)


With the format string you're using, this note isn't _as_ necessary.  (You 
could remove it.)  Ditto with the other similar comments.



support/apply-reviews.py (line 19)


Note: This would also resolve the issue I had with this line in the 
previous review.



support/apply-reviews.py (line 41)


Seems like this could go into the previous review.  Same for the next 2 
commenting changes/additions.


- Joseph Wu


On Oct. 22, 2015, 11:18 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> ---
> 
> (Updated Oct. 22, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38883/diff/
> 
> 
> Testing
> ---
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 38705: Added support for applying a review chain (apply-reviews.py).

[Vinod Kone]

> On Oct. 22, 2015, 9:20 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, line 25
> > 
> >
> > s/extract_//
> 
> Artem Harutyunyan wrote:
> Marco commented earlier on this one `nit: you are 'masking' the global 
> builtin id() here - that's a PEP8 violation, could you please rename to 
> something like rid?`.

is the concern that if you call this method review_id(), you cannot use 
"review_id" as a variable in the method? if yes, you can call the local 
variable 'rid'.


- Vinod


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


On Oct. 23, 2015, 6:16 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> ---
> 
> (Updated Oct. 23, 2015, 6:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38705/diff/
> 
> 
> Testing
> ---
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 38883: Removed calls to apply-review.sh script. Added support for amending commit messages.

[Vinod Kone]

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



support/apply-reviews.py (line 41)


capitalization of "Review ID" is different from above. please stick with 
one.



support/apply-reviews.py (lines 56 - 57)


bad rebase?



support/apply-reviews.py (line 78)


s/options/dry_run/

you can generalize it to 'options' later if there is a need for it.



support/apply-reviews.py (line 96)


ditto. just take 'dry_run' as an argument.



support/apply-reviews.py (line 113)


do you still need this if it's already in the atexit handler?



support/apply-reviews.py (lines 116 - 132)


i'm not sure these need to be functions. you can just inline them.



support/apply-reviews.py (line 134)


s/populate_//



support/apply-reviews.py (line 157)


seems weird that a function called amend() checks to see if it can amend or 
not. i would've imagined the caller to do that.



support/apply-reviews.py (line 173)


typically "-n" is used as a short argument for dry run. but i see why 
couldn't use it :(


- Vinod Kone


On Oct. 23, 2015, 6:18 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> ---
> 
> (Updated Oct. 23, 2015, 6:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
> https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38883/diff/
> 
> 
> Testing
> ---
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 39534: SSLTest refactor: Change MesosTest to inherit from the SSL helper class.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39495, 39498, 39499, 39501, 39533, 39534]

All tests passed.

- Mesos ReviewBot


On Oct. 26, 2015, 10:37 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39534/
> ---
> 
> (Updated Oct. 26, 2015, 10:37 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3762
> https://issues.apache.org/jira/browse/MESOS-3762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/39534/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure
> make check
> ```
> 
> ```
> ../configure --enable-ssl --enable-libevent
> make check
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39417: Add --egress_flow_classifier_parent flag

[Ian Downes]

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


Also, is there are way to test the different code paths?

- Ian Downes


On Oct. 17, 2015, 5:29 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39417/
> ---
> 
> (Updated Oct. 17, 2015, 5:29 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When --egress_unique_flow_per_container is enabled, we need to install a flow 
> classifier (fq_codel) qdisc on egress side. This flag specifies where to 
> install it in the hierarchy. By default, we install it at root. But, for 
> example, if you have already installed HTB qdisc at root, you may want this 
> to be installed in other place than root, specify an HTB class ID as its 
> parent here.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 
> 
> Diff: https://reviews.apache.org/r/39417/diff/
> 
> 
> Testing
> ---
> 
> Manual tests, with and without a pre-installed HTB qdisc and classes.
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Vinod Kone]

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

Ship it!



src/slave/validation.hpp (line 19)


__SLAVE_VALIDATION_HPP__



src/slave/validation.hpp (line 31)


why space here?



src/slave/validation.cpp (line 25)


kill new line.



src/slave/validation.cpp (line 65)


space before 'does'



src/slave/validation.cpp (line 75)


also print status.source()



src/tests/executor_http_api_tests.cpp (line 502)


kill this?



src/tests/executor_http_api_tests.cpp (line 504)


make sure this pattern is consistent everywhere else in this file.


- Vinod Kone


On Oct. 27, 2015, 12:23 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Oct. 27, 2015, 12:23 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e6169a0 
>   src/Makefile.am e797dac 
>   src/slave/http.cpp 3f7f71b 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38844: Added unit tests for Call validation in Agent

[Isabel Jimenez]

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

(Updated Oct. 27, 2015, 1:30 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

changes after review comments


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


Repository: mesos


Description
---

Unit tests for Call validation in Agent.


Diffs (updated)
-

  src/tests/executor_http_api_tests.cpp e429d84 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

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

(Updated Oct. 27, 2015, 3:42 a.m.)


Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.


Changes
---

moved untar to after all the downloads are done.


Repository: mesos


Description
---

Integrated remote puller with store. Tests will follow.


Diffs (updated)
-

  src/CMakeLists.txt 98e76cee81ab206f3ffe7989711abc38f49c4352 
  src/Makefile.am 96ce73b301c55d23bf4a5292e3d028148426a878 
  src/slave/containerizer/provisioner/docker/local_puller.hpp 
4574e8a04663482625d7b54f765741f221ec13e0 
  src/slave/containerizer/provisioner/docker/local_puller.cpp 
74d0e1ead7d630e65a7e75cb6123139b9197efef 
  src/slave/containerizer/provisioner/docker/puller.hpp 
105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
  src/slave/containerizer/provisioner/docker/puller.cpp 
cb05324689ffa26ce830b513e2d71b55517da3cb 
  src/slave/containerizer/provisioner/docker/registry_client.hpp 
1d3377e7462908246dfac90aa0c56314406529c9 
  src/slave/containerizer/provisioner/docker/registry_client.cpp 
471783d88b73b62afacac3d7952ebb5d5f442097 
  src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp 
637c97c0973bda492826803a962c99635647692d 
  src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
  src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 
  src/tests/containerizer/provisioner_docker_tests.cpp 
822aa77d0be0797ab62a5798527618b2cc4f6bf0 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38844: Added unit tests for Call validation in Agent

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38618, 38577, 38844]

All tests passed.

- Mesos ReviewBot


On Oct. 27, 2015, 1:30 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38844/
> ---
> 
> (Updated Oct. 27, 2015, 1:30 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unit tests for Call validation in Agent.
> 
> 
> Diffs
> -
> 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38844/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Anand Mazumdar]

> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/slave/validation.cpp, line 75
> > 
> >
> > also print status.source()
> 
> Isabel Jimenez wrote:
> We don't have a stringify for this.

Why not implement the `ostream` operator preferrably in `type_utils.hpp` in a 
separate review and mark this one as dependent on it ? Otherwise, this error 
string is hardly of any use to the end-user. What do you think?


- Anand


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


On Oct. 27, 2015, 1:20 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Oct. 27, 2015, 1:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e6169a0 
>   src/Makefile.am e797dac 
>   src/slave/http.cpp 3f7f71b 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39400: Quota: Implemented quota API.

[Guangya Liu]

> On 十月 25, 2015, 9:06 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 903
> > 
> >
> > I see that in the code, some are using 
> > 
> > '" << xxx << "'";
> > 
> > while some others are using
> > 
> > \"" << xxx << "\"";
> > 
> > What is the standard for different style?
> > 
> > Also when shall we need to add "" for one parameter in log message?
> 
> Alexander Rukletsov wrote:
> I think we are inconsistent. AFAIK, @neilc is doing some cleanup around.

https://reviews.apache.org/r/39497 has been merged, it is using {{'" << xxx << 
"'";}}, do we need to update this patch accordingly?


- Guangya


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


On 十月 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39400/
> ---
> 
> (Updated 十月 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39400/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39604: Added function that verifies prerequisites for using Linux launcher.

[Jie Yu]

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

Ship it!


Thanks!


src/slave/containerizer/linux_launcher.hpp (line 39)


Cna you rename it to 'available'? We tend to omit 'is' or 'are' for those 
functions (e.g., cgroups::enabled(), not cgroups::areEnabled()).


- Jie Yu


On Oct. 23, 2015, 11:48 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39604/
> ---
> 
> (Updated Oct. 23, 2015, 11:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Joris Van Remoortere, and Kapil 
> Arya.
> 
> 
> Bugs: MESOS-3800
> https://issues.apache.org/jira/browse/MESOS-3800
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We recently switched to using LinuxLauncher by default 
> (https://github.com/apache/mesos/blame/master/src/slave/containerizer/mesos/containerizer.cpp#L217-L234).
>  This causes problems with running the slave in a Docker container. 
> 
> This patch introduces a function for checking whether the machine is suited 
> for running Linux launcher. For the time being it checks whether the freezer 
> cgroup subsystem enabled, as the Linux launcher evolves we might want to add 
> more checks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/linux_launcher.hpp 
> 35b3315498d690ed66616617aa7d51455371fb5b 
>   src/slave/containerizer/linux_launcher.cpp 
> c03b89eb0678825b03a052874d6262f377a39e13 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d1fc5a460e7313828014eea999cf4e63dde01921 
> 
> Diff: https://reviews.apache.org/r/39604/diff/
> 
> 
> Testing
> ---
> 
> - Ran Mesoss tests in a Docker container where cgroup was not available.
> - Ran the new Jenkins script (https://reviews.apache.org/r/37787/).
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 39645: Updates /site to reflect Niklas' 0.25.0 changes that weren't reflected in git.

[Adam B]

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

Ship it!


Thanks! Make ReviewBot happy, and then I'm happy with this too.


site/source/blog/2015-10-12-mesos-0-25-0-released.md (line 37)


ReviewBot seems to be complaining about this new blank line. I know it's in 
the original from Nik's blog, but let's remove it before committing to git.


- Adam B


On Oct. 26, 2015, 1:46 a.m., Dave Lester wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39645/
> ---
> 
> (Updated Oct. 26, 2015, 1:46 a.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updates /site to reflect Niklas' 0.25.0 changes that weren't reflected in git.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2015-10-12-mesos-0-25-0-released.md PRE-CREATION 
>   site/source/downloads.html.md 49827fff0dedebc4738afbd02f17d746de72b5d6 
>   site/source/index.html.md b43325ab14c79f6b27db49fd52ea14d8d77c46c6 
> 
> Diff: https://reviews.apache.org/r/39645/diff/
> 
> 
> Testing
> ---
> 
> The website was built locally using rake, and these changes were reflected 
> properly.
> 
> 
> Thanks,
> 
> Dave Lester
> 
>

Re: Review Request 39496: Clarified libevent config error messages.

[Joris Van Remoortere]

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

Ship it!


This file is also copied in 3rdparty/libprocess, so we need to make these 
changes twice. I did this in a follow up patch.


configure.ac (line 1485)


none of the error messages append `library`, likely as it's implied by the 
prefix `lib`. Let's leave it off here to stay consistent.



configure.ac (line 1487)


How about `version 2+`?


- Joris Van Remoortere


On Oct. 20, 2015, 9:49 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39496/
> ---
> 
> (Updated Oct. 20, 2015, 9:49 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Neil Conway.
> 
> 
> Bugs: MESOS-3501
> https://issues.apache.org/jira/browse/MESOS-3501
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified libevent config error messages to include required version.
> 
> 
> Diffs
> -
> 
>   configure.ac 66f9b32773861d2921d99189e0fbcaea48c456a9 
> 
> Diff: https://reviews.apache.org/r/39496/diff/
> 
> 
> Testing
> ---
> 
> `../configure --enable-libevent --enable-ssl` with the libevent 
> libraries/headers in various states of disrepair.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39590: Made license-headers doxygen-compatible.

[Alexander Rukletsov]

> On Oct. 24, 2015, 7:23 p.m., Alexander Rukletsov wrote:
> > What about `.proto` files?
> 
> Benjamin Bannier wrote:
> Good point, since they contain C++ embedded they should follow a similar 
> style. I pushed an updated RR.
> 
> Most files follow that format; I cross-checked with a WIP linter check, 
> https://gist.github.com/bbannier/02231787b107b71360f7.

I think you also want to update description.


- Alexander


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


On Oct. 25, 2015, 2:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39590/
> ---
> 
> (Updated Oct. 25, 2015, 2:49 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Bernd 
> Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit is the result of running
> 
> for f in `find . -name '*.hpp' -or -name '*.cpp'`; do
>   sed -i -E '1 s~^\/\*\*~\/\*~' $f
> done
> 
> and updating the C++ style guide.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 0b6189174a4f0f1815625f68fb1a743b04a9cdad 
>   include/mesos/attributes.hpp 78afcd51b22a9eeb741076a8affeb8f2ae4bdee3 
>   include/mesos/authentication/authenticatee.hpp 
> 17fb7aa0f4d8a43f9cee0aab305af05f4fa7888f 
>   include/mesos/authentication/authentication.hpp 
> 699aa886286bc7d9c05592e71232ab1c1084871f 
>   include/mesos/authentication/authentication.proto 
> a7db56d643aa01ca2e3cd3e4268bd75b54136727 
>   include/mesos/authentication/authenticator.hpp 
> aa3818c31fee3b2e7c17d80dcceb8d41a38bbd06 
>   include/mesos/authorizer/authorizer.hpp 
> d667a52f90f970a313580446a5a006cec4b5e25b 
>   include/mesos/authorizer/authorizer.proto 
> 86bbb45f9d91b4098a262e3e50a793f3bb39497e 
>   include/mesos/containerizer/containerizer.hpp 
> 9bf76e066f390c36392c469b3d2cd92e2d10f3c7 
>   include/mesos/containerizer/containerizer.proto 
> 7cf6d2bb8c6636cbb3ea8c44fb45a41b93c8603d 
>   include/mesos/executor.hpp 72eca97dd84fb1300b37764a3ef3a57fb5e676c2 
>   include/mesos/executor/executor.hpp 
> 85f181c72cdb5e80d6c549a4d663d9bad261693a 
>   include/mesos/executor/executor.proto 
> a9243c7a10c32f104c12cbce835bc23a7c75a275 
>   include/mesos/fetcher/fetcher.hpp b7e6a717ed329d7f2586607cfe90342a96ae14a8 
>   include/mesos/fetcher/fetcher.proto 
> 1b2f4936d8dcd2b5e6d3ca2c85b3fa9df74a5797 
>   include/mesos/hook.hpp 6d7fee85566cf6c057296b5f4a9c14c9fab31085 
>   include/mesos/http.hpp 8b9b748fee5b2a8cc2261456cd6a74ebf9313164 
>   include/mesos/maintenance/maintenance.hpp 
> f676d01c2c81250b6e4740ab0934f966b50ed76d 
>   include/mesos/maintenance/maintenance.proto 
> aaca2513e2c30297bf624a831f5aa135f9f47e48 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   include/mesos/master/allocator.proto 
> 224da71e9f34d2ea11a6e6e235d0f8196abaeb90 
>   include/mesos/master/quota.hpp 5f7822f40af6fb23cdafdd0c205bcdc67e596935 
>   include/mesos/master/quota.proto d2e3a45735e4ebcf257682556aff5075e6e3bf79 
>   include/mesos/mesos.hpp 371d14a6cf802d1099be84f217a1af6554cc4eee 
>   include/mesos/mesos.proto 94004343ea615d87d7c7d63a6a811cf8002e 
>   include/mesos/module.hpp 01dd713941d504c3dfe606bfdf346d4702dc1495 
>   include/mesos/module/allocator.hpp 376eb4860322582f911d7a07253b79b1c9aa9292 
>   include/mesos/module/anonymous.hpp 22862bdba93537b7524f3143884b4d13d17c604a 
>   include/mesos/module/authenticatee.hpp 
> aafca1214cfdecb0479e4e8ab20fe9ffc8272473 
>   include/mesos/module/authenticator.hpp 
> b57938f8f4c5603b8e8e6d2e77f27a5eb302e99b 
>   include/mesos/module/authorizer.hpp 
> 7d8fc2123ac4132a7a698c855db03433eb77cea6 
>   include/mesos/module/hook.hpp fdbc5b19fe04ac9456b4141d673d9fec03e9c70d 
>   include/mesos/module/isolator.hpp 347d6d442debc70ff8accc4d89c944c2a2b7 
>   include/mesos/module/module.hpp 6ef106ee6f1559f8e95b8309f36af2b2d6e2c48b 
>   include/mesos/module/module.proto 821fc0e72ece7c497595859fc5efc1c64ea49b9b 
>   include/mesos/module/qos_controller.hpp 
> 462f07608fb2b580cee9548b5506e9896ee7077a 
>   include/mesos/module/resource_estimator.hpp 
> c1e42b97d831093bb92f8666fdfd53c8a9cae83f 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/scheduler.hpp f571d42d1508632152473c4f4ade60ae3900fce1 
>   include/mesos/scheduler/scheduler.hpp 
> 30de72b1b535831e074cd132b61e74fec2f4890a 
>   include/mesos/scheduler/scheduler.proto 
> 417cfe6a9bca418b0ef77cb2268fafeb07867819 
>   include/mesos/slave/isolator.hpp ea14bff0d31ffc440b0451675bfa440e09a524d8 
>   include/mesos/slave/isolator.proto 

Re: Review Request 39400: Quota: Implemented quota API.

[Alexander Rukletsov]

> On Oct. 25, 2015, 9:06 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 903
> > 
> >
> > I see that in the code, some are using 
> > 
> > '" << xxx << "'";
> > 
> > while some others are using
> > 
> > \"" << xxx << "\"";
> > 
> > What is the standard for different style?
> > 
> > Also when shall we need to add "" for one parameter in log message?

I think we are inconsistent. AFAIK, @neilc is doing some cleanup around.


- Alexander


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


On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39400/
> ---
> 
> (Updated Oct. 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39400/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Klaus Ma]

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



src/master/allocator/mesos/hierarchical.cpp (line 1004)


Just check the code, dynamically reserved resource are included in 
allocation.


- Klaus Ma


On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 24, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Qian Zhang]

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


For this patch, it seems that we add the code related to quota support in the 
slave foreach loop in the HierarchicalAllocatorProcess::allocate(const 
hashset& slaveIds_) method, so that means for **each slave**, we 
handle quota first and then the existing DRF fair share. I think there might be 
an issue for this approach: let say for the first slave, its available 
unreserved non-revocable resources can not satisfy a role’s quota due to the 
framework in this role has a filter for this slave, and then we lay aside the 
filtered resources of this slave for this role immediately. I think it might be 
too early for doing this since the other slaves may have resources which can 
satisfy this role’s quota. But if we lay aside this slave's resource for this 
role at this point, then the result is the framework of this role will not use 
these resources (due to the filter) AND all other role’s frameworks can not be 
offered with these resources too, this is kind of wasting resources.

I think maybe we can handle this quota support in this way: In 
HierarchicalAllocatorProcess::allocate(const hashset& slaveIds_), 
leave the existing 3 levels foreach loops (slave/role/framework) as they are, 
and add the quota related code separately before them in this way: traverse all 
quota’ed roles, for each of them, traverse all the slaves, and allocate each 
slave’s available unreserved non-revocable resources to the role’s framework 
(take filter and suppress into account) until the role’s quota is satisfied. 
After all the quota’ed role has been traversed, if there are still some role’s 
quotas are not satisfied, then lay aside resources (should be the resources 
filtered or suppressed) for them. In this way, before laying aside resources, 
we have tried our best to use all slave's the available resources to satisfy 
the quotas first, there should be less resources wasted.

- Qian Zhang


On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 24, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Qian Zhang]

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



src/master/allocator/mesos/hierarchical.cpp (line 1156)


I think we need to check if this is a quota'ed role first.


- Qian Zhang


On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 24, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Klaus Ma]

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



src/master/allocator/mesos/hierarchical.cpp (line 1018)


Can we move this out of the for loop? if the role is satisfied, framewor 
sort is not necessary.



src/master/allocator/mesos/hierarchical.cpp (line 1056)


s/overcimmittment/overcommittment



src/master/allocator/mesos/hierarchical.cpp (line 1094)


`allocable(...)` checked whether resource is enough to allocate (cpu > 
MIN_CPU && men > MIN_MEN). So if the resources can not allocate here, is also 
can not allocate in DRF stage.



src/master/allocator/mesos/hierarchical.cpp (line 1101)


My understading of this code is to lay aside resources for unsatisfaied 
Quota in next allocation cycle, right?
If so, I think we can move this out to another `foreach slaveIds` loop, so 
we did not need to lay aside resources.


- Klaus Ma


On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 24, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Artem Harutyunyan]

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



src/master/allocator/mesos/hierarchical.cpp (line 998)


s/dedicatedsorter/dedicated sorter/



src/master/allocator/mesos/hierarchical.cpp (line 1015)


s/sort/sorts/?



src/master/allocator/mesos/hierarchical.cpp (line 1026)


s/the/that/?



src/master/allocator/mesos/hierarchical.cpp (lines 1044 - 1046)


s/the/that/

Seems like a copy of the comment above, is this intentional?



src/master/allocator/mesos/hierarchical.cpp (line 1056)


s/overcimmittment/overcommitment of/

or 

s/overcimmittment/overcommitting/



src/master/allocator/mesos/hierarchical.cpp (line 1057)


s/guarantee/a guarantee/

It'd be great to expand this comment a bit and describe what the gurantee 
represents.


- Artem Harutyunyan


On Oct. 23, 2015, 9:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 23, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Alexander Rukletsov]

> On Oct. 26, 2015, 1:51 p.m., Artem Harutyunyan wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1044-1046
> > 
> >
> > s/the/that/
> > 
> > Seems like a copy of the comment above, is this intentional?

Yep, in both cases we have to laid aside resources.


> On Oct. 26, 2015, 1:51 p.m., Artem Harutyunyan wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1057
> > 
> >
> > s/guarantee/a guarantee/
> > 
> > It'd be great to expand this comment a bit and describe what the 
> > gurantee represents.

Do you think this is a good place for that?


- Alexander


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


On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Alexander Rukletsov]

> On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 996
> > 
> >
> > These newly added code makes allocate() a huge method (more than 200 
> > lines), maybe move these codes into a separate method?

Absolutely! The reason why it's not done is because we have already planned 
(but not yet scheduled) an allocator refactoring. Let me add a `TODO` for now 
in order to increase the pressure on ourselves ; ).


> On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1017-1020
> > 
> >
> > Why do we put these code inside the framework sorters foreach loop? I 
> > do not see it is related to framework.
> > If we really want to put these code here, then I think we also need to 
> > recalculate roleAllocatedResources every time when we allocate some 
> > resources to a framework of the role, and once the quota for the role is 
> > satifised, break.

There can be multiple frameworks in a role, hence quota may get satisfied after 
we allocate resources to some frameworks.

> then I think we also need to recalculate roleAllocatedResources every time 
> when we allocate some resources to a framework

But we do that at the end of the loop, right?


> On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1055-1057
> > 
> >
> > Since we know the guarentee of each quota'ed role and the gap between 
> > it and role's current allocation, I think it might be better to do the 
> > "find-grained" allocation, i.e., only allocate resources to fill the gap. 
> > Otherwise, we may run into the situation that we allocate too much resource 
> > to satisfy a role's guarentee but another role's guarentee can not be 
> > satisfied.

Yep, this can be the case and it was a hot debate during the design phase. I 
have decided to choose this approach because it's follows the least surprise 
principle for people who already familiar with DRF. I think we'll have to 
revisit the strategy anyway and (most probably) introduce `Quota.limit`. I 
think this is fine for MVP.


> On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1058
> > 
> >
> > We also have this code ```offerable[frameworkId][slaveId] = 
> > resources;``` in the following DRF allocation stage, so that means for a 
> > framework, the resources we allocate to it here will be overwritten by the 
> > the resources we allocate to it in DRF allocation stage? This seems not 
> > correct, maybe change the code in DRF allocation stage from "=" to "+="?

Good catch!


> On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1097
> > 
> >
> > If we *continue* from here, that means the following DRF allocation 
> > stage will be skipped? I think that is not what we want. So I guess it 
> > should be:
> > ```
> > if (allocatable(resources)) {
> >   // Lay aside the resources.
> >   laidAsideResources[slaveId] = resources;
> >   slaves[slaveId].allocated += resources;
> > }
> > ```

I think you're right.


- Alexander


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


On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Alexander Rukletsov]

> On Oct. 26, 2015, 2 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1018
> > 
> >
> > Can we move this out of the for loop? if the role is satisfied, 
> > framewor sort is not necessary.

And what if it is not but it got satisfied after we allocated resources to 
some—but not all!—frameworks in this role?


> On Oct. 26, 2015, 2 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1094
> > 
> >
> > `allocable(...)` checked whether resource is enough to allocate (cpu > 
> > MIN_CPU && men > MIN_MEN). So if the resources can not allocate here, is 
> > also can not allocate in DRF stage.

But `resources` may differ: during DRF a framework may get some revocable 
resources which may render the whole offer allocatable.


> On Oct. 26, 2015, 2 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1101
> > 
> >
> > My understading of this code is to lay aside resources for unsatisfaied 
> > Quota in next allocation cycle, right?
> > If so, I think we can move this out to another `foreach slaveIds` loop, 
> > so we did not need to lay aside resources.

Im not sure I got your proposal. We should bookkeep resources that are 
"reserved" or "laid aside" as part of role's quota in order not to offer them 
to anybody else as non-revocable. However, we may want to offer them as 
revocable offers.


- Alexander


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


On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Alexander Rukletsov]

> On Oct. 26, 2015, 1:13 p.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1156
> > 
> >
> > I think we need to check if this is a quota'ed role first.

Correct, thanks!


- Alexander


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


On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Alexander Rukletsov]

> On Oct. 26, 2015, 1:49 p.m., Qian Zhang wrote:
> > For this patch, it seems that we add the code related to quota support in 
> > the slave foreach loop in the HierarchicalAllocatorProcess::allocate(const 
> > hashset& slaveIds_) method, so that means for **each slave**, we 
> > handle quota first and then the existing DRF fair share. I think there 
> > might be an issue for this approach: let say for the first slave, its 
> > available unreserved non-revocable resources can not satisfy a role’s quota 
> > due to the framework in this role has a filter for this slave, and then we 
> > lay aside the filtered resources of this slave for this role immediately. I 
> > think it might be too early for doing this since the other slaves may have 
> > resources which can satisfy this role’s quota. But if we lay aside this 
> > slave's resource for this role at this point, then the result is the 
> > framework of this role will not use these resources (due to the filter) AND 
> > all other role’s frameworks can not be offered with these resources too, 
> > this is kind of wasting resource
 s.
> > 
> > I think maybe we can handle this quota support in this way: In 
> > HierarchicalAllocatorProcess::allocate(const hashset& slaveIds_), 
> > leave the existing 3 levels foreach loops (slave/role/framework) as they 
> > are, and add the quota related code separately before them in this way: 
> > traverse all quota’ed roles, for each of them, traverse all the slaves, and 
> > allocate each slave’s available unreserved non-revocable resources to the 
> > role’s framework (take filter and suppress into account) until the role’s 
> > quota is satisfied. After all the quota’ed role has been traversed, if 
> > there are still some role’s quotas are not satisfied, then lay aside 
> > resources (should be the resources filtered or suppressed) for them. In 
> > this way, before laying aside resources, we have tried our best to use all 
> > slave's the available resources to satisfy the quotas first, there should 
> > be less resources wasted.

I'm not sure I got your point. If my mental compiler is correct, if a framework 
in quota'ed role opts out, we do not immediately lay aside resources. We do 
that after we have checked all the frameworks in the role in a separate loop.


- Alexander


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


On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39494: Added libnl, libevent and ssl flags to config docs.

[Joris Van Remoortere]

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

Ship it!


Thanks for taking this on Greg!
Greg will follow up with some patches to make the style in this document 
consistent. We've kept it locally consistent for now.


docs/configuration.md (lines 1659 - 1660)


`the libevent development package is required`?



docs/configuration.md (line 1660)


`version 2+`



docs/configuration.md (line 1670)


`--enable-libevent is currently required`?



docs/configuration.md (line 1888)


It seems like there are 2 forms of this flag / message:
`--with-libevent[=DIR]` and ``--with-libevent=[=DIR]``

It seems like there are 2 forms of this message:
```
excludes building and using the bundled XXX package in lieu of an installed 
version at a location prefixed by the given path
```
and what you have.
Feel free to make them totally consistent with a new review.

Here and below.


- Joris Van Remoortere


On Oct. 21, 2015, 5:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39494/
> ---
> 
> (Updated Oct. 21, 2015, 5:28 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-3749
> https://issues.apache.org/jira/browse/MESOS-3749
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added libnl, libevent and ssl flags to config docs.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c7d5da68a5cede7a8d57ccf94cebf8a10af2d9c6 
> 
> Diff: https://reviews.apache.org/r/39494/diff/
> 
> 
> Testing
> ---
> 
> Viewed in the Mesos Website Container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Alexander Rukletsov]

> On Oct. 25, 2015, 1:39 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 982
> > 
> >
> > s/unsatisfiedRoleQuotas/unAllocatedRoleQuotas?

Dynamic reservations may not be allocated, but should still count towards quota.


> On Oct. 25, 2015, 1:39 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1035
> > 
> >
> > I know that we have design to exclue the reserved resource from quota, 
> > but why not include the current role's reserved as available?

If you mean dynamically reserved resources than yes, we can do that. If you 
mean statically reserved than nope, we decided not to conflate them with quota.


> On Oct. 25, 2015, 1:39 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1058
> > 
> >
> > Where does those offerable resource offered to master?

https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L956

Dropping the issue, feel free to reopen if I haven't answered your question.


> On Oct. 25, 2015, 1:39 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1074
> > 
> >
> > s/unsatisfiedRoleQuota/unAllocatedQuota?

See above.


> On Oct. 25, 2015, 1:39 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1102
> > 
> >
> > Why adding those resource as allocated? The `resource` here is 
> > unAllocated resource, why adding it to allocated?

Later on, during the DRF stage we do some resource math, which should be 
correct: 
https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L910


- Alexander


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


On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Oct. 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39360: Relocate MesosContainerizer specific files to the correct location

[Jie Yu]

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

Ship it!


This looks good! You missed launcher and linux_launcher (those are mesos 
containerizer specific). For the rest, it looks good. I'll commit this first 
and you can follow up with a patch to move launcher and linux_launcher.

- Jie Yu


On Oct. 20, 2015, 10 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39360/
> ---
> 
> (Updated Oct. 20, 2015, 10 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3129
> https://issues.apache.org/jira/browse/MESOS-3129
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Relocate MesosContainerizer specific files to the correct location
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 98cbafc134ec388a176d50172912fbfdf9f5bfa3 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/examples/test_isolator_module.cpp 
> 577dfcac260b4f5df7ab4e9599e4caac46ccd1e1 
>   src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d 
>   src/slave/containerizer/isolators/cgroups/constants.hpp  
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> 54b83a7d67f9cacbca4f9dd9b9b72a3dbc2e5263 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> ba748c6caec7253b42167e8a4f9b4535da858259 
>   src/slave/containerizer/isolators/cgroups/mem.hpp  
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> 55fa6f4019e1521dd816138e82db110d573ae6b8 
>   src/slave/containerizer/isolators/cgroups/perf_event.hpp  
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 03035dfbfb84470ba39ed9ecfd1eb73890e3f784 
>   src/slave/containerizer/isolators/filesystem/linux.hpp 
> 93e85f2aa7bfceb7e55ff33bdc2e0e0a5cb8f880 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 8823b7850a1ac17fc4f4868aadf1b04428d2381b 
>   src/slave/containerizer/isolators/filesystem/posix.hpp  
>   src/slave/containerizer/isolators/filesystem/posix.cpp 
> eec510c4f7655d67b33ad90210eeb57fcc910684 
>   src/slave/containerizer/isolators/filesystem/shared.hpp  
>   src/slave/containerizer/isolators/filesystem/shared.cpp 
> 73804ca5a8a3bf03e13c74a247b5c21e9af5f040 
>   src/slave/containerizer/isolators/namespaces/pid.hpp  
>   src/slave/containerizer/isolators/namespaces/pid.cpp 
> a9823e08b195b8df82de2a7b410a4e6ef99f8853 
>   src/slave/containerizer/isolators/network/helper.cpp 
> e5fb99e87ac16150b85b1c6f6965681f7fe77ce0 
>   src/slave/containerizer/isolators/network/port_mapping.hpp  
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef 
>   src/slave/containerizer/isolators/posix.hpp  
>   src/slave/containerizer/isolators/posix/disk.hpp  
>   src/slave/containerizer/isolators/posix/disk.cpp 
> 73e62a225da062733557287afa2273d8183d76fd 
>   src/slave/containerizer/linux_launcher.cpp 
> c03b89eb0678825b03a052874d6262f377a39e13 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d1fc5a460e7313828014eea999cf4e63dde01921 
>   src/slave/containerizer/provisioner/appc/paths.hpp  
>   src/slave/containerizer/provisioner/appc/paths.cpp 
> 8817c0ff4b6806f08afd322e250a9a53b7b0a5d6 
>   src/slave/containerizer/provisioner/appc/spec.hpp  
>   src/slave/containerizer/provisioner/appc/spec.cpp 
> bbe523d2ee1dd558cc5007e578cbf23abac8e1de 
>   src/slave/containerizer/provisioner/appc/store.hpp 
> e8455197dcc3f4c9856db20605f6862b8755a946 
>   src/slave/containerizer/provisioner/appc/store.cpp 
> a5ef4ea7cd08423360120430833c5881053637f5 
>   src/slave/containerizer/provisioner/backend.hpp  
>   src/slave/containerizer/provisioner/backend.cpp 
> b5d96701ae6bd49365b169f4e5150b8c4dae1870 
>   src/slave/containerizer/provisioner/backends/bind.hpp 
> 1685938fb4349e790b9595cc4c67584c7f31a392 
>   src/slave/containerizer/provisioner/backends/bind.cpp 
> 1fe1746c0bc1c9c12e1378e6438122a91b58316b 
>   src/slave/containerizer/provisioner/backends/copy.hpp 
> 7a5aaa41d8f6842ef437ed7a34235d8baac4bfff 
>   src/slave/containerizer/provisioner/backends/copy.cpp 
> 92fb0988da0bdd5a2b5a5f53ab61b7bb19c61cda 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp 
> 4574e8a04663482625d7b54f765741f221ec13e0 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp 
> 74d0e1ead7d630e65a7e75cb6123139b9197efef 
>   src/slave/containerizer/provisioner/docker/message.hpp 
> 466e6f838d143917fa7eebb13b0a670a6b80117c 
>   src/slave/containerizer/provisioner/docker/message.proto  
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> 885080dbd3603f8c71ac867b88edcfd22276567f 
>   

Re: Review Request 39539: Windows: Moved `os::mktemp` to its own file, `stout/os/mktemp.hpp`.

[Joris Van Remoortere]

> On Oct. 23, 2015, 4:49 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, lines 270-274
> > 
> >
> > Do you need this one?

There are some uses of `os::mktemp` in the code-base.


> On Oct. 23, 2015, 4:49 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, lines 280-284
> > 
> >
> > Why is this function body different than all the other alias-like 
> > functions?
> > 
> > i.e. You could comment that stout/mktemp needs an open file, or similar.

Agree with Joseph.
This is not obvious, had to look at the documentation for linux's mkstemp().


- Joris


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


On Oct. 22, 2015, 3:35 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39539/
> ---
> 
> (Updated Oct. 22, 2015, 3:35 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-3441
> https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Moved `os::mktemp` to its own file, `stout/os/mktemp.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> ba2836a72ceee948cb43364e80ada9f132f33d04 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 7f70c9ea7d57634b5bfd40523ba37561ec92a09a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/mktemp.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 7a8819541506f57f70a9b577dc97a76fc26ebaa8 
> 
> Diff: https://reviews.apache.org/r/39539/diff/
> 
> 
> Testing
> ---
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>