Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 1600
> > 
> >
> > Great to see a default QoS controller!

Thx, can i `fix` that? =DDD


- Bartek


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


On Dec. 10, 2015, 5:03 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 5:03 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote:
> > src/slave/qos_controllers/load.hpp, lines 39-45
> > 
> >
> > Awesome comment! Let's make it a doxygen style one.

Hmm, i followed all mesos style guidelines e.g 
https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md.. And 
mimic other class' comments.. Not sure how to improve that (:


> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote:
> > src/slave/qos_controllers/load.cpp, lines 126-142
> > 
> >
> > This is a bit dense block; mind breaking it up a bit?

Done.


> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote:
> > src/slave/qos_controllers/load.cpp, line 224
> > 
> >
> > Let's remove the trailing '.'
> > Maybe we can include some guidance on where to set the thresholds (in 
> > the module parameters). No biggie if you don't want that in.

Good point. Let's make it consistent with other Mesos messages. (:


> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 81
> > 
> >
> > Why this removal?

There was sth there i removed some reviews ago... and accidenlty remove that 
line too. Fixed.


> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 1127
> > 
> >
> > Can we wrap this in a smart pointer? Is there a potential race between 
> > the test code and the load qos controller when changing the values of the 
> > stub load, or how do we guarantee that?

I think i've found even more clean solution - moved to regular instance and 
just pust the reference to lambda. (:


> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, lines 1143-1150
> > 
> >
> > Wonder if we can reduce some of this boiler plate; have you taken a 
> > look at `createTasks()`?

Unfortunately, `createTasks` uses offers to create tasks, and beside that it 
creates tasks _NOT_ `ResourceUsage::Executor`. I cleaned it and made separate 
inline function for that. Hope it helped.


- Bartek


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


On Dec. 10, 2015, 5:03 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 5:03 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 40567: Windows: 1/3 Added zlib definitions for Windows.

[Dario Bazan]

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

(Updated Dec. 10, 2015, 6:07 p.m.)


Review request for mesos and Alex Clemmer.


Repository: mesos


Description
---

Windows: 1/3 Added zlib definitions for Windows.


Diffs (updated)
-

  3rdparty/cmake/Versions.cmake 932f2f66b04e5ca3d2ed04da1e7019d2ff7488e4 
  cmake/CompilationConfigure.cmake ab503b23f054ebc9a3877a3eca27b1b4190aa51b 

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


Testing
---


Thanks,

Dario Bazan

Re: Review Request 41158: Turn off rx checksum offloading for veth in container

[Cong Wang]

> On Dec. 10, 2015, 12:27 a.m., Jie Yu wrote:
> > This is more like a question: do we need to turn off tx side as well?

This is not needed, because 1) the physical interface can finally checksum it 
after it moves out of the container to the gateway interface; 2) if the 
physcial interface is not able to do it, the kernel can do it right before 
delivering it.


- Cong


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


On Dec. 9, 2015, 11:15 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41158/
> ---
> 
> (Updated Dec. 9, 2015, 11:15 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Bugs: MESOS-4105
> https://issues.apache.org/jira/browse/MESOS-4105
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We noticed that in some cases we delivered some corrupt packets to 
> applications running in our containers. This is clearly wrong. 
> 
> Here is what happens:
> 
> 1) We receive a corrupt packet externally
> 2) The hardware driver is able to checksum it and notices it has a bad 
> checksum
> 3) The driver delivers this packet anyway to wait for TCP layer to checksum 
> it again and then drop it
> 4) This packet is moved to a veth interface because it is for a container
> 5) Both sides of the veth pair have RX checksum offloading by default
> 6) The veth_xmit() marks the packet's checksum as UNNECESSARY since its peer 
> device has rx checksum offloading
> 7) Packet is moved into the container TCP/IP stack
> 8) TCP layer is not going to checksum it since it is not necessary
> 9) The packet gets delivered to application layer
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 89bb36f936417de8169a2442729fbd7c9d60acb7 
> 
> Diff: https://reviews.apache.org/r/41158/diff/
> 
> 
> Testing
> ---
> 
> 1) Turn rx checksum off manually and the bug is gone
> 2) Test this patch and verify rx checksum is turned off as expected.
> 3) I don't see any noticable performance issue after turning this off
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 39782: Add a comment for os::libraries::setPaths.

[James Peach]

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

(Updated Dec. 10, 2015, 6:46 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


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


Repository: mesos


Description
---

Add a comment for os::libraries::setPaths.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
03e6f75850561b5eb92da4771fbe18e4057ad520 

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


Testing
---

No code changes.


Thanks,

James Peach

Re: Review Request 41211: Aggregated Http as member in Master.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Dec. 10, 2015, 5:32 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41211/
> ---
> 
> (Updated Dec. 10, 2015, 5:32 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before this change the Http object was copied in to each HTTP routing
> lambda. If state were to be introduced, this could create hard to
> debug issues. Aggregating a single instance as a member of Master
> prevents these issues and simplifies access to communal code paths
> such as `authenticate`.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7 
> 
> Diff: https://reviews.apache.org/r/41211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 41209: Replace Docker with MockDocker in HealthCheckTest.

[Mesos ReviewBot]

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


Bad review!

Reviews applied: []

Error:
 No reviewers specified. Please find a reviewer by asking on JIRA or the 
mailing list.

- Mesos ReviewBot


On Dec. 10, 2015, 5:10 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41209/
> ---
> 
> (Updated Dec. 10, 2015, 5:10 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace Docker with MockDocker in HealthCheckTest.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 0fbccc373204d3b9431c614bdd6d046cc07e4566 
> 
> Diff: https://reviews.apache.org/r/41209/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38649: Add a benchmark to simulate frameworks declining offers.

[Joris Van Remoortere]

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

Ship it!


Thanks for cleaning this up James!
I've touched up a few things that I pointed out below.


src/tests/hierarchical_allocator_tests.cpp (line 82)


we use camelCase in Mesos.
I know it's confusing as we use snake_case in libprocess.
One day we will clean this up :-)



src/tests/hierarchical_allocator_tests.cpp (line 113)


same as above



src/tests/hierarchical_allocator_tests.cpp (line 1293)


increased this to 1hr.



src/tests/hierarchical_allocator_tests.cpp (lines 1295 - 1296)


touched up.



src/tests/hierarchical_allocator_tests.cpp (lines 1311 - 1326)


cleaned up variable naming.



src/tests/hierarchical_allocator_tests.cpp (line 1315)


removed.



src/tests/hierarchical_allocator_tests.cpp (lines 1344 - 1345)


indentation.



src/tests/hierarchical_allocator_tests.cpp (line 1346)


we use a new line after statements that do not fit on a single line.



src/tests/hierarchical_allocator_tests.cpp (lines 1368 - 1374)


We try to use full words for variable names.


- Joris Van Remoortere


On Oct. 29, 2015, 10:57 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38649/
> ---
> 
> (Updated Oct. 29, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3493
> https://issues.apache.org/jira/browse/MESOS-3493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This benchmark starts a number of slaves and frameworks, then cycles
> the allocator. On each allocation pass, the frameworks decline all
> the offers. This leads to increasing numbers of refusal filters in
> the allocator, and the allocation slows down. After around 200
> cycles the slowdown increases out of proportion.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 505b9de3d8d888c296f6103c80fe9f0ef1c2ca16 
> 
> Diff: https://reviews.apache.org/r/38649/diff/
> 
> 
> Testing
> ---
> 
> make check && make bench
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Vinod Kone]

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



src/slave/qos_controllers/load.cpp (line 76)


s/Cannot/Failed to/



src/slave/qos_controllers/load.cpp (line 77)


kill this.



src/slave/qos_controllers/load.cpp (line 81)


s/overLoaded/overloaded/



src/slave/qos_controllers/load.cpp (line 102)


we prefer colons for internal protobufs

s/ResourceUSage_Executor/ResourceUsage::Executor/



src/slave/qos_controllers/load.cpp (lines 103 - 106)


I think you can just say "Set kill correction for all revocable executors." 
at the top instead of these 2 comments.



src/slave/qos_controllers/load.cpp (line 126)


why protected instead of private?



src/slave/qos_controllers/load.cpp (line 190)


LOG(ERROR) << "Failed to parse 5min load threshold: " << 
thresholdParam.error();
return NULL;



src/slave/qos_controllers/load.cpp (line 191)


just return NULL here. i think we should error out even if one of the 
thresholds is wrong.



src/slave/qos_controllers/load.cpp (line 199)


ditto. see above.



src/slave/qos_controllers/load.cpp (lines 205 - 210)


if you do `return`s above, this could just be else block

```
} else {
  LOG(ERROR) << "No load thresholds are configured for LoadQoSController";
  return NULL;
}

```



src/tests/oversubscription_tests.cpp (line 161)


it's hard to tell what this method is doing.

i would rather make this a function that returns ExecutorInfo.

```
ExecutorInfo createExecutorInfo(
const string& executorId,
const string& frameworkId
)
```

do you really need these executors to have different `source`s?



src/tests/oversubscription_tests.cpp (line 1136)


If "the" total system load on "the" agent exceeds "the" configured 
threshold...



src/tests/oversubscription_tests.cpp (lines 1150 - 1152)


// Prepare stubbed `os::Load` whose values are below the thresholds.


- Vinod Kone


On Dec. 10, 2015, 5:03 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 5:03 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 41113: Added `IsolatorRecoveryInfo` message as the sole parameter to `Isolator::recover()`.

[Neil Conway]

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


Re: "facilating the addition of new parameters without breaking the interface", 
is the goal to ensure ABI compatibility or API compatibility? If the former, a 
protobuf is not sufficient (you typically need to use the PIML idiom or 
something similar).

- Neil Conway


On Dec. 9, 2015, 11:59 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41113/
> ---
> 
> (Updated Dec. 9, 2015, 11:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Neil Conway.
> 
> 
> Bugs: MESOS-4003
> https://issues.apache.org/jira/browse/MESOS-4003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the message `IsolatorRecoveryInfo` and makes it the sole 
> parameter of `Isolator::recover()`, in order to facilitate the future 
> addition of parameters without breaking the interface.
> 
> In addition to the two existing parameters of `Isolator::recover()`, a third 
> member was added to `IsolatorRecoveryInfo`: the agent's `work_dir`. This is 
> useful for the network storage DVD isolator 
> (https://github.com/emccode/mesos-module-dvdi) in particular.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp 95a2933988ea7c9b9404df5e12031f134712b2b5 
>   include/mesos/slave/isolator.proto d2032adf9336119ed8e1ff3c813d657d70331b67 
>   src/common/protobuf_utils.hpp 7280c9fe36726df6b02ff468c7bd5ecedf5f5023 
>   src/common/protobuf_utils.cpp 6e1eb0b8465809d1da5dac1cd29b692b9fa6ff66 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6dad2e858b68cf47e048d49d34af4fa4cb3b6841 
>   src/slave/containerizer/mesos/isolator.hpp 
> 937f253656d36ed10b47ceeb0b6101f212e65586 
>   src/slave/containerizer/mesos/isolator.cpp 
> 493b5dd26cf0e8f986381a502cfa6d1dde6573d4 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
> 123b9ed3ccaebcd5da24fc62ff7a92d4a81ed760 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
> 3b95e195ad704f163c245175390d9a26bde7e17c 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.hpp 
> 09952369c72d3c6322ae7a1c73cd68226d452ad2 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.cpp 
> 2ddb9f4adbb879682cd39966ab974cf3fa32209c 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
> 5eaf49f1f35c93ad4465adb6c9c9cf57b3a2c6ee 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
> b7ba00bc495001380f01737e46e8671ffe1c2ef7 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> b8d47e8250a892fa333a0a966a0f38fe1f2816f2 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8442e9c30612fa04f34130b9f967cb1414880ca6 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
> c3544aa313cbb185efb03bba59961cdf2b616a37 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> 00ff84b6cd0aa29fa5a7918d7f88d480af8752ca 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.hpp 
> 2e457015a0348a457581edf493877b71fab17090 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> 361ed6561bd5e2f75d026922def01f42b43d61c2 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
> c2d1455249618f9cd2e17dc2244b184d52b32eaf 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> d65c1593b44f4b21237581147e57e441ebf3160d 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> cbb94077d46d7b87ffc09b72e02269bc16f25f92 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 89bb36f936417de8169a2442729fbd7c9d60acb7 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 7e1ebc2fada5a5e291e84c7044bdba9a71f4b42c 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> 31808c1e8199fbf2cea36c273860fdbf0a2388f8 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> d971db09083faad08f3cf18c25a79245321d1d9a 
>   src/tests/containerizer/isolator.hpp 
> e4101b188560bd857ea104f61f52f27c880e7731 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fe679354d04d68b68e168cd8c4eab23898f6532f 
> 
> Diff: https://reviews.apache.org/r/41113/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` was run on OSX and Ubuntu in order to test both the 
> posix and linux isolator code. Only expected test failures were observed. 
> Some of the failures on Ubuntu are `SlaveRecoveryTest`s, which is a bit 
> disconcerting, but this issue is tracked in MESOS-4025, and seems to be due 
> to artifacts left behind by previous tests. If I do:
> 
> `GTEST_FILTER="" make check`
> `sudo GTEST_FILTER="SlaveRecoveryTest*" bin/mesos-tests.sh`
> 
> then all the `SlaveRecoveryTest`s pass.
> 
> 
> Thanks,
> 
> 

Review Request 41211: Aggregated Http as member in Master.

[Joris Van Remoortere]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Before this change the Http object was copied in to each HTTP routing
lambda. If state were to be introduced, this could create hard to
debug issues. Aggregating a single instance as a member of Master
prevents these issues and simplifies access to communal code paths
such as `authenticate`.


Diffs
-

  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40539, 37999, 38000, 38094, 38950, 39043]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 2:34 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> ---
> 
> (Updated Dec. 10, 2015, 2:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
>   src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 41215: Added test case for quota behavior in the presence of empty roles.

[Neil Conway]

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

Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


Repository: mesos


Description
---

i.e., roles with no frameworks currently registered.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 40632: WIP: Enabled oversubscribed resources for reservations in allocator.

[Guangya Liu]

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

(Updated 十二月 10, 2015, 8:31 a.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
and Klaus Ma.


Repository: mesos


Description
---

WIP: Enabled oversubscribed resources for reservations in allocator.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
5da825a1d578a9ee40b4985378fddb3c5fb3b416 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 40872: Changed RegistryClientTest to use instance work directory.

[Timothy Chen]

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



src/tests/containerizer/provisioner_docker_tests.cpp (line 527)


Btw, we use camel case for method names.


- Timothy Chen


On Dec. 2, 2015, 4:23 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40872/
> ---
> 
> (Updated Dec. 2, 2015, 4:23 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClientTest suite uses static variable for is work directory. This has
> been changed to model the TemporaryDirectoryTest suite.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40872/diff/
> 
> 
> Testing
> ---
> 
> make check (600 times).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41158: Turn off rx checksum offloading for veth in container

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41158]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 9, 2015, 11:15 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41158/
> ---
> 
> (Updated Dec. 9, 2015, 11:15 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Bugs: MESOS-4105
> https://issues.apache.org/jira/browse/MESOS-4105
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We noticed that in some cases we delivered some corrupt packets to 
> applications running in our containers. This is clearly wrong. 
> 
> Here is what happens:
> 
> 1) We receive a corrupt packet externally
> 2) The hardware driver is able to checksum it and notices it has a bad 
> checksum
> 3) The driver delivers this packet anyway to wait for TCP layer to checksum 
> it again and then drop it
> 4) This packet is moved to a veth interface because it is for a container
> 5) Both sides of the veth pair have RX checksum offloading by default
> 6) The veth_xmit() marks the packet's checksum as UNNECESSARY since its peer 
> device has rx checksum offloading
> 7) Packet is moved into the container TCP/IP stack
> 8) TCP layer is not going to checksum it since it is not necessary
> 9) The packet gets delivered to application layer
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 89bb36f936417de8169a2442729fbd7c9d60acb7 
> 
> Diff: https://reviews.apache.org/r/41158/diff/
> 
> 
> Testing
> ---
> 
> 1) Turn rx checksum off manually and the bug is gone
> 2) Test this patch and verify rx checksum is turned off as expected.
> 3) I don't see any noticable performance issue after turning this off
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 39848: Validate revocable resources

[Guangya Liu]

> On 十二月 5, 2015, 12:21 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1503
> > 
> >
> > I think at this point it's worthwhile to factor out the validations in 
> > this function into slave/validation.cpp file like we have done in 
> > master/validation.cpp. The validation should likely return an object 
> > (TaskStatus?) that contains the message and REASON to be used in the status 
> > update.
> > 
> > This would also help with unit testing the validations more easily.

OK, will factor out the validations to slave/validation.cpp. Can you please 
help check if the logic is OK here? As I was using one double value to minus 
another, shall we use CHECK_NEAR as the solution in 
https://reviews.apache.org/r/40767/diff/1#index_header ? Thanks!


- Guangya


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


On 十一月 5, 2015, 7:36 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated 十一月 5, 2015, 7:36 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40532: Added notion of evictable task to RunTaskMessage.

[Guangya Liu]

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

(Updated 十二月 10, 2015, 8:34 a.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
and Klaus Ma.


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


Repository: mesos


Description
---

Added notion of evictable task to RunTaskMessage.


Diffs (updated)
-

  src/messages/messages.proto 178d757a76f14829da6daab97ec678843717cf5a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 40873: RegistryClientTests: Created separate server to serve blobs.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Dec. 3, 2015, 5:20 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> ---
> 
> (Updated Dec. 3, 2015, 5:20 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By having a separate blob server, it simulates the real world more closely. 
> It also allows the test server to be in accept mode early.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40873/diff/
> 
> 
> Testing
> ---
> 
> make check (600 count).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41078: Fixed tests to call socket accept before sending response.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Dec. 8, 2015, 7:03 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41078/
> ---
> 
> (Updated Dec. 8, 2015, 7:03 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By calling accept before sending a response, the server would be ready for
> client's next request.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/41078/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 40872: Changed RegistryClientTest to use instance work directory.

[Timothy Chen]

> On Dec. 10, 2015, 8:37 a.m., Timothy Chen wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, line 526
> > 
> >
> > How about just use os::getcwd? And why do we even need a uuid based 
> > working directory if tmp directory test creates a directory already?

For now I'll remove this and just use os::getcwd as I don't really see it being 
necessary.


- Timothy


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


On Dec. 2, 2015, 4:23 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40872/
> ---
> 
> (Updated Dec. 2, 2015, 4:23 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClientTest suite uses static variable for is work directory. This has
> been changed to model the TemporaryDirectoryTest suite.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40872/diff/
> 
> 
> Testing
> ---
> 
> make check (600 times).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > Are you also planning to update 
> > https://github.com/apache/mesos/blob/master/docs/oversubscription.md ?

Already in review: https://reviews.apache.org/r/41042/  (:


Thx for detailed review i will fix the issues soon.


> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 1602-1603
> > 
> >
> > how come the fixed estimator only has a source file and this one has a 
> > header too?

I needed Load QoS controller interface (in header file) for the unit tests in 
`oversubscription_tests.hpp`. Fixed estimator was tested as integration test. 
(Loaded as module and checking status update message).


> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 1674
> > 
> >
> > do we need this? i don't see slave/resource_estimators/fixed.cpp here?

See comment above. I need that for my unit tests. Fixed estimator has only 
integration test.


- Bartek


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


On Dec. 8, 2015, 11:16 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 8, 2015, 11:16 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 40872: Changed RegistryClientTest to use instance work directory.

[Timothy Chen]

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



src/tests/containerizer/provisioner_docker_tests.cpp (line 526)


How about just use os::getcwd? And why do we even need a uuid based working 
directory if tmp directory test creates a directory already?


- Timothy Chen


On Dec. 2, 2015, 4:23 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40872/
> ---
> 
> (Updated Dec. 2, 2015, 4:23 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClientTest suite uses static variable for is work directory. This has
> been changed to model the TemporaryDirectoryTest suite.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40872/diff/
> 
> 
> Testing
> ---
> 
> make check (600 times).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41212: Adjust timeout value in HealthCheckTest.CheckCommandTimeout.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41212]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 5:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41212/
> ---
> 
> (Updated Dec. 10, 2015, 5:37 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Bugs: MESOS-4024
> https://issues.apache.org/jira/browse/MESOS-4024
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adjust timeout value in HealthCheckTest.CheckCommandTimeout.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 0fbccc373204d3b9431c614bdd6d046cc07e4566 
> 
> Diff: https://reviews.apache.org/r/41212/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39410: Added support for github to apply-reviews.py.

[Artem Harutyunyan]

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

(Updated Dec. 10, 2015, 1:35 p.m.)


Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco 
Massenzio, and Vinod Kone.


Changes
---

Renamed variables to be more consistent.


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


Repository: mesos


Description
---

Added support for github to apply-reviews.py.


Diffs (updated)
-

  support/apply-reviews.py 6ea229facebf6ee071c0d0186fd1f702e0c48a39 

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


Testing
---

Tested with python 2.7


Thanks,

Artem Harutyunyan

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

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



src/master/http.cpp (line 1588)


AlexR suggested including roles that have any reserved resources here. That 
makes sense, but AFAIK there isn't an easy way to find this information (unless 
we want to iterate over all the slaves and examine their resources). Thoughts?


- Neil Conway


On Dec. 10, 2015, 10:09 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 10, 2015, 10:09 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yongqiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> e239b4746494fcc2b362a83afb634a2ce5e25f9b 
>   src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39410: Added support for github to apply-reviews.py.

[Artem Harutyunyan]

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

(Updated Dec. 10, 2015, 1:42 p.m.)


Review request for mesos, Adam B, Joris Van Remoortere, Joseph Wu, Marco 
Massenzio, and Vinod Kone.


Changes
---

Fixed a typo.


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


Repository: mesos


Description
---

Added support for github to apply-reviews.py.


Diffs (updated)
-

  support/apply-reviews.py 6ea229facebf6ee071c0d0186fd1f702e0c48a39 

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


Testing
---

Tested with python 2.7


Thanks,

Artem Harutyunyan

Re: Review Request 40553: Enable mesos tests installation

[James Peach]

> On Dec. 10, 2015, 3:30 p.m., Till Toenshoff wrote:
> > src/tests/utils.cpp, lines 71-77
> > 
> >
> > I still don't understand why the oversubscription tests need to have 
> > this installed in the LIBDIR and not in MODULE_DIR folder. We need to find 
> > some proper reasoning here before making so much extra fuzz for this, I 
> > feel.

I made ``PKGMODULEDIR`` a first class installation directory and installed 
``lib_fixed_resource_estimator`` there (with compatibility symlinks). This let 
me clean up and remove ``getModuleDir``.


> On Dec. 10, 2015, 3:30 p.m., Till Toenshoff wrote:
> > configure.ac, line 330
> > 
> >
> > Could you please explain the [1] for the negative case?

The ```[1]``` is the argument to ```AC_DEFINE``` not to the ```AS_IF```. I'll 
split the line to make it clearer.


> On Dec. 10, 2015, 3:30 p.m., Till Toenshoff wrote:
> > src/tests/containerizer/memory_test_helper.cpp, line 187
> > 
> >
> > Can you explain briefly what you mean here by "too many dependencies" - 
> > seemed to me we only "drag" in "stout/json.hpp"?!

I took another look and I just need to link against 
``$(mesos_tests_DEPENDENCIES)``.


> On Dec. 10, 2015, 3:30 p.m., Till Toenshoff wrote:
> > src/tests/module_tests.cpp, lines 60-68
> > 
> >
> > Seems this could be covered by `getModulePath`, no?

Yeh I can use ``getModulePath`` here. I think I was trying to minimize the 
changes :)


- James


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


On Nov. 30, 2015, 6:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40553/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Bugs: MESOS-3608
> https://issues.apache.org/jira/browse/MESOS-3608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables the installation mesos-tests and its dependencies
> and helper tool. The goal is to allow operators to build a separate
> test package that can be run at deployment time to verify that Mesos
> works in the deployment environment.
> 
> Since the build directory is searched first, to run it on a host
> that has a build tree, you need to specify a non-existent tree:
> 
> ~ $ $PREFIX/libexec/mesos/tests/mesos-tests --build_dir=/none
> 
> - Add --enable-tests-install
> - Fix mesos-tests gmock dependencies
> - Optionally install tests, helpers and test modules
> - Add utility helpers to find various test resources
> 
> Current test status:
> 
>   [==] 784 tests from 106 test cases ran. (135304 ms total)
>   [  PASSED  ] 780 tests.
>   [  FAILED  ] 4 tests, listed below:
>   [  FAILED  ] ExamplesTest.TestFramework
>   [  FAILED  ] ExamplesTest.NoExecutorFramework
>   [  FAILED  ] ExamplesTest.EventCallFramework
>   [  FAILED  ] ExamplesTest.PersistentVolumeFramework
> 
> 
> Diffs
> -
> 
>   configure.ac b30a8d30076f3068fd7d5fc8ccea1982639e998a 
>   src/Makefile.am fd38cfa73d81a98c819378f99a766e2ddb7e1a04 
>   src/tests/containerizer/launch_tests.cpp 
> c7ebe2606e4ff99ced90342dd16e0b4bf02bc504 
>   src/tests/containerizer/memory_test_helper.cpp 
> 4a3de2e3c887aa6afc604588850e1386f92d8c11 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fe679354d04d68b68e168cd8c4eab23898f6532f 
>   src/tests/containerizer/ns_tests.cpp 
> 603e54b7303c5aa15e2c5715dc7a2f7e7d39541b 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 
>   src/tests/environment.cpp 36f0ad0b739559e5c883d72585731944da4283cf 
>   src/tests/fetcher_tests.cpp 99c494a2167ea9ec94e63d92980df17a4089f95a 
>   src/tests/health_check_tests.cpp b1454b085b36bb7c4d8ef012c764cd8466b4fb02 
>   src/tests/mesos.cpp a75182ec7f6d0c6063bbb36a6ccf4e8a4a81c8dd 
>   src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 
>   src/tests/module_tests.cpp b5cfbcd4885a4f2c20b19e00958fedda81afa6e0 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
>   src/tests/script.cpp ee44fef29fb40e414d7507168091ee5cd0d15736 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
>   src/tests/utils.hpp a6cca472f4dfab12cd6eccab6972206d842177aa 
>   src/tests/utils.cpp 877139e97249761658dce3b1058cdc2e2a52367b 
> 
> Diff: https://reviews.apache.org/r/40553/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> 

Review Request 41225: Added test cases for implicit roles.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

Added test cases for implicit roles.


Diffs
-

  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Neil Conway

Review Request 41223: Cleaned up creation of HTTP auth headers in tests.

[Neil Conway]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

Cleaned up creation of HTTP auth headers in tests.


Diffs
-

  src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de 
  src/tests/mesos.hpp 2429ac5cbcd9c1a3949c11de94b542108a3c13d8 
  src/tests/persistent_volume_endpoints_tests.cpp 
0a03b5f1ac7dec14bd99c31768f86100f2b60616 
  src/tests/reservation_endpoints_tests.cpp 
d5d2aa7c203aa7357b564ff51cd3b38230195d04 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 10, 2015, 10:13 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 367-368
> > 
> >
> > I'm not sure how badly we need to clean up this map. What happens if we 
> > leave a role in the activeRoles list even after a framework unregisters? 
> > Will it make the roleSorter noticeably slower?

I haven't tested the performance implications either way, but keeping the map 
accurate seems like it is more readable/easier for the programmer to reason 
about. Can you elaborate on your concern with cleaning up the map?


> On Dec. 10, 2015, 10:13 a.m., Adam B wrote:
> > src/master/master.hpp, line 2052
> > 
> >
> > Seems strange that you wouldn't replace this with a `Role(string name, 
> > double weight)` constructor. You complain about Role not knowing its 
> > name/weight elsewhere.

I did this to avoid redundancy: we're already storing the name of the role (as 
the map key), and the role's weight is stored in a separate map.


- Neil


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


On Dec. 9, 2015, 5:54 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 9, 2015, 5:54 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yongqiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40553: Enable mesos tests installation

[James Peach]

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

(Updated Dec. 10, 2015, 9:28 p.m.)


Review request for mesos, Benjamin Bannier and Till Toenshoff.


Changes
---

Rebased onto master (+ other review branches). Addressed review comments.


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


Repository: mesos


Description
---

This patch enables the installation mesos-tests and its dependencies
and helper tool. The goal is to allow operators to build a separate
test package that can be run at deployment time to verify that Mesos
works in the deployment environment.

Since the build directory is searched first, to run it on a host
that has a build tree, you need to specify a non-existent tree:

~ $ $PREFIX/libexec/mesos/tests/mesos-tests --build_dir=/none

- Add --enable-tests-install
- Fix mesos-tests gmock dependencies
- Optionally install tests, helpers and test modules
- Add utility helpers to find various test resources

Current test status:

  [==] 784 tests from 106 test cases ran. (135304 ms total)
  [  PASSED  ] 780 tests.
  [  FAILED  ] 4 tests, listed below:
  [  FAILED  ] ExamplesTest.TestFramework
  [  FAILED  ] ExamplesTest.NoExecutorFramework
  [  FAILED  ] ExamplesTest.EventCallFramework
  [  FAILED  ] ExamplesTest.PersistentVolumeFramework


Diffs (updated)
-

  configure.ac b30a8d30076f3068fd7d5fc8ccea1982639e998a 
  src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  src/tests/containerizer/launch_tests.cpp 
c7ebe2606e4ff99ced90342dd16e0b4bf02bc504 
  src/tests/containerizer/memory_test_helper.cpp 
4a3de2e3c887aa6afc604588850e1386f92d8c11 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
fe679354d04d68b68e168cd8c4eab23898f6532f 
  src/tests/containerizer/ns_tests.cpp 603e54b7303c5aa15e2c5715dc7a2f7e7d39541b 
  src/tests/containerizer/port_mapping_tests.cpp 
9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 
  src/tests/environment.cpp 36f0ad0b739559e5c883d72585731944da4283cf 
  src/tests/fetcher_tests.cpp 1831d896ca8a52bec4adf87a67b6af845079796c 
  src/tests/health_check_tests.cpp 0fbccc373204d3b9431c614bdd6d046cc07e4566 
  src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 
  src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 
  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
  src/tests/oversubscription_tests.cpp 0333281c247dd182860a49f39be791c00679bf6b 
  src/tests/script.cpp ee44fef29fb40e414d7507168091ee5cd0d15736 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
  src/tests/utils.hpp a6cca472f4dfab12cd6eccab6972206d842177aa 
  src/tests/utils.cpp 877139e97249761658dce3b1058cdc2e2a52367b 

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


Testing
---


Thanks,

James Peach

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 10, 2015, 10:08 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yongqiao 
Wang.


Changes
---

Rebase, new tests.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  src/tests/reservation_tests.cpp 54a1b23b22087b5152825125ba146b4cc47af88d 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 41225: Added test cases for implicit roles.

[Neil Conway]

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

(Updated Dec. 10, 2015, 11:07 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Yongqiao Wang.


Changes
---

Improve a test case.


Repository: mesos


Description
---

Added test cases for implicit roles.


Diffs (updated)
-

  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 40532: Added notion of evictable task to RunTaskMessage.

[Klaus Ma]

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

Ship it!


Ship It!

- Klaus Ma


On Dec. 10, 2015, 10:26 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40532/
> ---
> 
> (Updated Dec. 10, 2015, 10:26 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
> and Klaus Ma.
> 
> 
> Bugs: MESOS-3890
> https://issues.apache.org/jira/browse/MESOS-3890
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added notion of evictable task to RunTaskMessage.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto b66bac14056407140bb3cf5aa3e67ac94df1a2f8 
> 
> Diff: https://reviews.apache.org/r/40532/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40375: Support distinguishing revocable resources in the Resource protobuf.

[Guangya Liu]

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


Can you please rebase this patch? I cannot apply this for test now due to code 
conflict. Thanks!

- Guangya Liu


On Dec. 1, 2015, 1:15 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40375/
> ---
> 
> (Updated Dec. 1, 2015, 1:15 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3888
> https://issues.apache.org/jira/browse/MESOS-3888
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3888: We need to distinguish revocable resource for usage slack and 
> allocation slack.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 27971fe 
>   include/mesos/v1/mesos.proto 9acefd5 
>   src/common/resources.cpp 98804a4 
>   src/tests/resources_tests.cpp dbd39cd 
>   src/v1/resources.cpp 8488c31 
> 
> Diff: https://reviews.apache.org/r/40375/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 40375: Support distinguishing revocable resources in the Resource protobuf.

[Klaus Ma]

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

(Updated Dec. 11, 2015, 9:21 a.m.)


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


Changes
---

rebase


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


Repository: mesos


Description
---

MESOS-3888: We need to distinguish revocable resource for usage slack and 
allocation slack.


Diffs (updated)
-

  include/mesos/mesos.proto 8ca2130 
  include/mesos/v1/mesos.proto 8f357b0 
  src/common/resources.cpp 5a79817 
  src/tests/resources_tests.cpp ce47bac 
  src/v1/resources.cpp d300842 

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


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 41158: Turn off rx checksum offloading for veth in container

[David Robinson]

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



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 1097 - 
1098)


This writes to stderr, which can end up in the logs.

[root@server ~]# ethtool --version 1> /dev/null
ethtool version 6
Usage:
ethtool DEVNAME Display standard information about device

Log snippet:

I1211 01:05:13.215730 10885 main.cpp:190] Build: 2015-12-10 22:54:33 by 
mockbuild
I1211 01:05:13.215859 10885 main.cpp:192] Version: 0.26.0-tw5
I1211 01:05:13.215996 10885 containerizer.cpp:142] Using isolation: 
cgroups/cpu,cgroups/mem,network/port_mapping,posix/disk,cgroups/perf_event,filesystem/posix
ethtool version 6
Usage:
ethtool DEVNAME Display standard information about device
I1211 01:05:13.251729 10885 port_mapping.cpp:1255] Using eth0 as the public 
interface
I1211 01:05:13.252707 10885 port_mapping.cpp:1280] Using lo as the loopback 
interface


- David Robinson


On Dec. 9, 2015, 11:15 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41158/
> ---
> 
> (Updated Dec. 9, 2015, 11:15 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Bugs: MESOS-4105
> https://issues.apache.org/jira/browse/MESOS-4105
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We noticed that in some cases we delivered some corrupt packets to 
> applications running in our containers. This is clearly wrong. 
> 
> Here is what happens:
> 
> 1) We receive a corrupt packet externally
> 2) The hardware driver is able to checksum it and notices it has a bad 
> checksum
> 3) The driver delivers this packet anyway to wait for TCP layer to checksum 
> it again and then drop it
> 4) This packet is moved to a veth interface because it is for a container
> 5) Both sides of the veth pair have RX checksum offloading by default
> 6) The veth_xmit() marks the packet's checksum as UNNECESSARY since its peer 
> device has rx checksum offloading
> 7) Packet is moved into the container TCP/IP stack
> 8) TCP layer is not going to checksum it since it is not necessary
> 9) The packet gets delivered to application layer
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 89bb36f936417de8169a2442729fbd7c9d60acb7 
> 
> Diff: https://reviews.apache.org/r/41158/diff/
> 
> 
> Testing
> ---
> 
> 1) Turn rx checksum off manually and the bug is gone
> 2) Test this patch and verify rx checksum is turned off as expected.
> 3) I don't see any noticable performance issue after turning this off
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 41223: Cleaned up creation of HTTP auth headers in tests.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41223]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 9:46 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41223/
> ---
> 
> (Updated Dec. 10, 2015, 9:46 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up creation of HTTP auth headers in tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de 
>   src/tests/mesos.hpp 2429ac5cbcd9c1a3949c11de94b542108a3c13d8 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 0a03b5f1ac7dec14bd99c31768f86100f2b60616 
>   src/tests/reservation_endpoints_tests.cpp 
> d5d2aa7c203aa7357b564ff51cd3b38230195d04 
> 
> Diff: https://reviews.apache.org/r/41223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 41234: Close leaked FD in RegistryClientTest.SimpleRegistryPuller.

[Joseph Wu]

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

Review request for mesos, Artem Harutyunyan, Jojy Varghese, and Timothy Chen.


Repository: mesos


Description
---

Close leaked FD in RegistryClientTest.SimpleRegistryPuller.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
3f1717b770e139c3759aab0aeda9dbcf5029b0c2 

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


Testing
---

make check 

bin/mesos-tests.sh --gtest_filter="RegistryClientTest.SimpleRegistryPuller" 
--gtest_repeat=256


Thanks,

Joseph Wu

Re: Review Request 41159: Corrected a comment in reservation endpoint tests.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Dec. 9, 2015, 11:22 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41159/
> ---
> 
> (Updated Dec. 9, 2015, 11:22 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> c3833d0949df42c9f8dadf3c6e6b0b49e6cbdce9 
> 
> Diff: https://reviews.apache.org/r/41159/diff/
> 
> 
> Testing
> ---
> 
> None.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41156: Employed a better macro in tests, cleaned up formatting.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Dec. 9, 2015, 11:36 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41156/
> ---
> 
> (Updated Dec. 9, 2015, 11:36 p.m.)
> 
> 
> Review request for mesos, Michael Park, Neil Conway, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced all instances of `AWAIT_EXPECT_EQ({true|false}, ...)` with more 
> appropriate `AWAIT_EXPECT_{TRUE|FALSE}(...)`. Also wrapped typenames in 
> backticks in "authorization_tests.cpp".
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 32beeea3584f093bcac24e0c160235de0e3abb28 
>   src/tests/group_tests.cpp 9db6162dae0a2657e12f4e99334f9cab65d14e9a 
> 
> Diff: https://reviews.apache.org/r/41156/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="*AuthorizationTest*:*GroupTest*" make check -j7` on Mac OS 
> 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Adam B]

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


Seems like the right overall approach, but I've realized that we need to be 
careful about breaking the Allocator module API. Put RoleInfo back and I'll do 
another pass.


src/master/allocator/mesos/allocator.hpp (line 58)


I don't think we need to break the allocator API for this.
Sorry for misleading you before, but RoleInfo (and everything in 
include/mesos/master/allocator.hpp) is a part of the allocator module 
interface, potentially in use by community allocator modules. So, we'd have to 
issue an API change proposal/notification if you're actually going to change 
the parameter type. We're running into the opposite issue now with the Isolator 
module, where we're breaking the API to introduce a protobuf 
IsolatorRecoveryInfo parameter just to hold work_dir. Once we have the 
protobuf, we can then add new optional parameters without breaking the API 
again.



src/master/allocator/mesos/hierarchical.hpp (lines 367 - 368)


I'm not sure how badly we need to clean up this map. What happens if we 
leave a role in the activeRoles list even after a framework unregisters? Will 
it make the roleSorter noticeably slower?



src/master/master.hpp (lines 1332 - 1333)


Would an `Option` make the `if(explicitRoles.isNone())` 
check more readable than `if(validRoles.empty())`?



src/master/master.hpp 


Seems strange that you wouldn't replace this with a `Role(string name, 
double weight)` constructor. You complain about Role not knowing its 
name/weight elsewhere.



src/master/master.cpp (lines 552 - 556)


We don't this kind of `*`-boxing for deprecation warnings.
We also don't usually get excited enough to use exclamation marks.



src/master/master.cpp (line 5534)


How about a CHECK << "message" explaining what's failing?



src/master/master.cpp (lines 5534 - 5538)


For how many times you say `framework->info.role()`, you should probably 
assign it to a local variable.



src/master/master.cpp (lines 5827 - 5830)


I guess with Role.RoleInfo or Role.Weights, you wouldn't delete the Role if 
it had a non-default weight.



src/master/quota_handler.cpp (line 303)


s/permitted/on the role whitelist, if it exists/
"permitted" sounds too much like ACL permissions to me.


- Adam B


On Dec. 8, 2015, 9:54 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161 
>   src/master/master.hpp 

Re: Review Request 41001: Improved 'ReservationTest.ACLMultipleOperations'.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Dec. 10, 2015, 2:02 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> ---
> 
> (Updated Dec. 10, 2015, 2:02 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved 'ReservationTest.ACLMultipleOperations'.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp 3405b47c367e3c20e2a9139ab95dd8fd1805ea8d 
> 
> Diff: https://reviews.apache.org/r/41001/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh 
> --gtest_repeat=1000 --gtest_break_on_failure`
> 
> This test was originally written to test the interaction of multiple 
> RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when 
> authorization is enabled. In order to probe the effect of authorization on 
> this interaction, some operations should fail due to failed authorization. 
> However, this test included operations that failed simply because they were 
> malformed. I altered the test so that now operations will fail due to failed 
> authorization, which tests the desired functionality more precisely.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 41185: CMake: Updated LFLAG for dl library to defined label

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41096, 41185]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 3:26 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41185/
> ---
> 
> (Updated Dec. 10, 2015, 3:26 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Updated LFLAG for dl library to defined label
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> 9893d741cd7c611dc65eba76be03e06dac618132 
> 
> Diff: https://reviews.apache.org/r/41185/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 40469: Update Allocator interface to support dynamic roles

[Adam B]

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


This looks like just the interface change; where's the (default/reference) 
implementation?
Justify/delete the removeRole call.
Consider (the lack of) backwards-compatibility for your allocator module API 
change.


include/mesos/master/allocator.hpp (line 409)


When is removeRole necessary? There aren't likely to be so many roles that 
we need to worry about saving memory by clearing out inactive roles.
If there are no frameworks registered with this role, it is inactive and 
won't affect allocator decisions.
If an admin wants to unspecify a weight for this user, they could just set 
it back to the default '1.0'



include/mesos/master/allocator.hpp (lines 412 - 418)


This is a breaking API change for the allocator, and any implementers of 
allocator modules will not only have to recompile their modules against the 
newer version, but will also have to implement this new function.

I wonder if it would be better to not make this a pure virtual function and 
instead have a default noop implementation, so module authors can recompile 
without error, and add dynamic role support to their own allocator at their 
leisure.

Either way, we'll need to make an announcement to the dev@ list and put a 
note in the upgrades doc.



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


Seems like there's a lot more missing here. Which subsequent review 
actually implements the new functions? Perhaps that patch should be merged into 
this one so we have a functional patch to commit at once.


- Adam B


On Dec. 7, 2015, 9:20 p.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40469/
> ---
> 
> (Updated Dec. 7, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3956
> https://issues.apache.org/jira/browse/MESOS-3956
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update Allocator interface to support dynamic roles
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
> 
> Diff: https://reviews.apache.org/r/40469/diff/
> 
> 
> Testing
> ---
> 
> Make check successfully.
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 40431: Move RoleInfo message out of allocator.proto

[Adam B]

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


I'm not sure exactly why you needed to move RoleInfo out of allocator.proto. 
The RoleInfo we use for the allocator module API doesn't need to be (and 
perhaps shouldn't be) the same class that we use to display role information 
like weights in the HTTP endpoint, or even the same class that we persist in 
the registry. There may be some future role metadata we want to set in the HTTP 
endpoint that doesn't need to be passed on to the allocator (e.g. 
Role.description), and maybe even something we set that doesn't need to be 
persisted. I don't actually think we need 3 or 4 separate RoleInfo-like 
protobufs, but I want us to think about how each of these APIs could grow apart 
in their notion of "important role metadata". Is there a real need to move the 
RoleInfo protobuf?


include/mesos/master/allocator.proto (line 19)


Shouldn't this file have `java_package` and `java_outer_classname` just 
like the other protos?
Looks like isolator.proto and oversubscription.proto are missing it too. 
Would you mind creating a separate patch to fix that?



include/mesos/role/role.proto (line 21)


Why change the package? Couldn't this still be in `mesos.master`? Then you 
wouldn't have to change all the other files.


- Adam B


On Dec. 7, 2015, 9:20 p.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40431/
> ---
> 
> (Updated Dec. 7, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3944
> https://issues.apache.org/jira/browse/MESOS-3944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently role protobuf is defined in allocator.proto due to only the 
> traditional DRF allocator uses roles as it’s first level of hierarchy, I 
> think we should move it out and define it in a separated file as quota had in 
> dynamic roles project, because role protobuf will also be used by master to 
> persist.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   include/mesos/role/role.hpp PRE-CREATION 
>   include/mesos/role/role.proto PRE-CREATION 
>   src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/40431/diff/
> 
> 
> Testing
> ---
> 
> 1. Make Check successfully;
> 
> 2. $ curl http://9.110.48.168:5050/roles
> {"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 41001: Improved 'ReservationTest.ACLMultipleOperations'.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40999, 41001]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 2:02 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> ---
> 
> (Updated Dec. 10, 2015, 2:02 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved 'ReservationTest.ACLMultipleOperations'.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp 3405b47c367e3c20e2a9139ab95dd8fd1805ea8d 
> 
> Diff: https://reviews.apache.org/r/41001/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh 
> --gtest_repeat=1000 --gtest_break_on_failure`
> 
> This test was originally written to test the interaction of multiple 
> RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when 
> authorization is enabled. In order to probe the effect of authorization on 
> this interaction, some operations should fail due to failed authorization. 
> However, this test included operations that failed simply because they were 
> malformed. I altered the test so that now operations will fail due to failed 
> authorization, which tests the desired functionality more precisely.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/tests/oversubscription_tests.cpp, lines 1163-1176
> > 
> >
> > push this inside the lamba below?

Can i ask why? (: From the computation perspective now i will create such 
structure only once and only copy, whereas in lambda we would create it in 
every lambda call


- Bartek


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


On Dec. 8, 2015, 11:16 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 8, 2015, 11:16 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Review Request 41212: Adjust timeout value in HealthCheckTest.CheckCommandTimeout.

[haosdent huang]

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

Review request for mesos, Ben Mahler and Timothy Chen.


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


Repository: mesos


Description
---

Adjust timeout value in HealthCheckTest.CheckCommandTimeout.


Diffs
-

  src/tests/health_check_tests.cpp 0fbccc373204d3b9431c614bdd6d046cc07e4566 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

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

Ship it!


Looking great! Will let Vinod have one more look, but otherwise looks good to 
go.


src/slave/qos_controllers/load.hpp (line 47)


Minor s/loadAvg/loadAverage/



src/tests/oversubscription_tests.cpp (line 1151)


Newline after


- Niklas Nielsen


On Dec. 10, 2015, 9:03 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 9:03 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Review Request 41216: Cleaned up DRF allocator tests.

[Neil Conway]

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

Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


Repository: mesos


Description
---

Cleaned up DRF allocator tests.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 41096: CMake: Added LFLAGs need for linux cmake build

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 10, 2015, 3:16 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41096/
> ---
> 
> (Updated Dec. 10, 2015, 3:16 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> a9cd9902567cef4c7ab4125463a68e19907db0b4 
> 
> Diff: https://reviews.apache.org/r/41096/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 41185: CMake: Updated LFLAG for dl library to defined label

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 10, 2015, 3:26 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41185/
> ---
> 
> (Updated Dec. 10, 2015, 3:26 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Updated LFLAG for dl library to defined label
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> 9893d741cd7c611dc65eba76be03e06dac618132 
> 
> Diff: https://reviews.apache.org/r/41185/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 41216: Cleaned up DRF allocator tests.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 10, 2015, 6:08 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41216/
> ---
> 
> (Updated Dec. 10, 2015, 6:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up DRF allocator tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41216/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

[James Peach]

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

(Updated Dec. 10, 2015, 6:45 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


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


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

> On Dec. 9, 2015, 10:32 a.m., Niklas Nielsen wrote:
> > src/slave/qos_controllers/load.hpp, lines 39-45
> > 
> >
> > Awesome comment! Let's make it a doxygen style one.
> 
> Bartek Plotka wrote:
> Hmm, i followed all mesos style guidelines e.g 
> https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md.. And 
> mimic other class' comments.. Not sure how to improve that (:

nvm - feel free to drop


- Niklas


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


On Dec. 10, 2015, 9:03 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 9:03 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

> On Dec. 9, 2015, 2:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 1600
> > 
> >
> > Great to see a default QoS controller!
> 
> Bartek Plotka wrote:
> Thx, can i `fix` that? =DDD

@vinod - maybe wasn't meant as an issue?


- Niklas


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


On Dec. 10, 2015, 9:03 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 9:03 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 40568: Windows: 2/3 Added zlib compilation steps for Windows.

[Dario Bazan]

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

(Updated Dec. 10, 2015, 6:12 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Windows: 2/3 Added zlib compilation steps for Windows.


Diffs
-

  3rdparty/libprocess/3rdparty/CMakeLists.txt 
9b61376ea6aad304607c20c9823d9ef19013eca0 
  3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
a9cd9902567cef4c7ab4125463a68e19907db0b4 
  3rdparty/libprocess/cmake/ProcessConfigure.cmake 
2a37fdb6501aaf7baac2ada0a714bbe67e7c5aca 
  3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
6eda46b0e08829b269b13a78f22510e6d03940d9 

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


Testing
---


Thanks,

Dario Bazan

Re: Review Request 41215: Added test case for quota behavior in the presence of empty roles.

[Joris Van Remoortere]

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

Ship it!



src/tests/hierarchical_allocator_tests.cpp (lines 1773 - 1774)


whitespace



src/tests/hierarchical_allocator_tests.cpp (lines 1802 - 1809)


clarified these comments a little.



src/tests/hierarchical_allocator_tests.cpp (lines 1815 - 1816)


whitespace.


- Joris Van Remoortere


On Dec. 10, 2015, 6:07 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41215/
> ---
> 
> (Updated Dec. 10, 2015, 6:07 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> i.e., roles with no frameworks currently registered.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41215/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39780: Update OversubscriptionTest to not assume dynamic dlopen search.

[James Peach]

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

(Updated Dec. 10, 2015, 6:44 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


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


Repository: mesos


Description
---

Update OversubscriptionTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/oversubscription_tests.cpp 0333281c247dd182860a49f39be791c00679bf6b 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 40431: Move RoleInfo message out of allocator.proto

[Yongqiao Wang]

> On Dec. 10, 2015, 10:40 a.m., Adam B wrote:
> > include/mesos/master/allocator.proto, line 19
> > 
> >
> > Shouldn't this file have `java_package` and `java_outer_classname` just 
> > like the other protos?
> > Looks like isolator.proto and oversubscription.proto are missing it 
> > too. Would you mind creating a separate patch to fix that?

I am not sure if we need to add java_package and java_outer_classname in those 
proto files, can you please clarify a little more about why we need to do this?


> On Dec. 10, 2015, 10:40 a.m., Adam B wrote:
> > include/mesos/role/role.proto, line 21
> > 
> >
> > Why change the package? Couldn't this still be in `mesos.master`? Then 
> > you wouldn't have to change all the other files.

Like other feature(such as quota), I also think role manamgnet is a seprated 
function, so I define role protobuf in a separated package rather than define 
it in mesos.proto.


- Yongqiao


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


On Dec. 8, 2015, 5:20 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40431/
> ---
> 
> (Updated Dec. 8, 2015, 5:20 a.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3944
> https://issues.apache.org/jira/browse/MESOS-3944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently role protobuf is defined in allocator.proto due to only the 
> traditional DRF allocator uses roles as it’s first level of hierarchy, I 
> think we should move it out and define it in a separated file as quota had in 
> dynamic roles project, because role protobuf will also be used by master to 
> persist.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   include/mesos/role/role.hpp PRE-CREATION 
>   include/mesos/role/role.proto PRE-CREATION 
>   src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/40431/diff/
> 
> 
> Testing
> ---
> 
> 1. Make Check successfully;
> 
> 2. $ curl http://9.110.48.168:5050/roles
> {"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 41209: Replace Docker with MockDocker in HealthCheckTest.

[haosdent huang]

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

(Updated Dec. 11, 2015, 3:59 a.m.)


Review request for mesos, Ben Mahler, Greg Mann, Jan Schlicht, and Timothy Chen.


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


Repository: mesos


Description
---

Replace Docker with MockDocker in HealthCheckTest.


Diffs
-

  src/tests/health_check_tests.cpp 0fbccc373204d3b9431c614bdd6d046cc07e4566 

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


Testing
---


Thanks,

haosdent huang

Review Request 41236: Some consistency style fixes in Docker puller.

[Jie Yu]

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

Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy 
Chen.


Repository: mesos


Description
---

Some consistency style fixes in Docker puller.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
4aa4a9c4074d96c30c3bceea59d071feeecae2ea 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 41237: Renamed a function parameter in Docker puller.

[Jie Yu]

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

Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy 
Chen.


Repository: mesos


Description
---

Renamed a function parameter in Docker puller.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
31bcc86e0e63e6535dcb62d61201c2ef310bf90d 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
4aa4a9c4074d96c30c3bceea59d071feeecae2ea 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 41225: Added test cases for implicit roles.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40995, 41075, 41225]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 11:07 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41225/
> ---
> 
> (Updated Dec. 10, 2015, 11:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for implicit roles.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41225/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 41235: Cleaned up the untar method in docker puller.

[Jie Yu]

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

Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy 
Chen.


Repository: mesos


Description
---

Cleaned up the untar method in docker puller.

The extra promise is not needed.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
4aa4a9c4074d96c30c3bceea59d071feeecae2ea 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 40469: Update Allocator interface to support dynamic roles

[Yongqiao Wang]

> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > This looks like just the interface change; where's the (default/reference) 
> > implementation?
> > Justify/delete the removeRole call.
> > Consider (the lack of) backwards-compatibility for your allocator module 
> > API change.

Refer to some other Epic(such as quota), they always split the changes into 
some smaller bits, so I also follow up them and want to implement these 
functions in another sepratated JIRA MESOS-3943, I think it is easier to 
reivew. I will post another patch for the implementation, is it OK?

For backwards-compatibility, I have considered this issue before, but I think 
Mesos does not reach to 1.0 so interfaces changes is resonable, and also refer 
to the quota implementation, the functions setQuota()/removeQuota() also be 
defined to pure virtual functions. I think we should keep consistence.


> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > include/mesos/master/allocator.hpp, line 409
> > 
> >
> > When is removeRole necessary? There aren't likely to be so many roles 
> > that we need to worry about saving memory by clearing out inactive roles.
> > If there are no frameworks registered with this role, it is inactive 
> > and won't affect allocator decisions.
> > If an admin wants to unspecify a weight for this user, they could just 
> > set it back to the default '1.0'

For Dynamic Roles, I think it is make sence to provide a way to let cluster 
operator to remove a role due to the corresponding way is provided to add a 
role by /roles endpoint. But for Implicit Roles, this is non-necessary, I will 
update this patch to remove this function.


> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > include/mesos/master/allocator.hpp, lines 412-418
> > 
> >
> > This is a breaking API change for the allocator, and any implementers 
> > of allocator modules will not only have to recompile their modules against 
> > the newer version, but will also have to implement this new function.
> > 
> > I wonder if it would be better to not make this a pure virtual function 
> > and instead have a default noop implementation, so module authors can 
> > recompile without error, and add dynamic role support to their own 
> > allocator at their leisure.
> > 
> > Either way, we'll need to make an announcement to the dev@ list and put 
> > a note in the upgrades doc.

I suggest to make this funciton as a pure virtual function and let some other 
allocator to implement it, my reasons as below:
1. Mesos does not reach to 1.0, so the interface changes are resonable.
2. Like quota configuration(those functions are also be defined as pure virtual 
funcs in allocator.hpp), Weight dynamic update also is an important functions, 
I think it should be required for any mesos cluster.


Of couse, It is great to define this function with a default noop 
implementation for backwards-compatibility, I just confused for the consistence 
like quota done(setQuota()/removeQuota()). OK, let me know your further comment.


> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1057-1058
> > 
> >
> > Seems like there's a lot more missing here. Which subsequent review 
> > actually implements the new functions? Perhaps that patch should be merged 
> > into this one so we have a functional patch to commit at once.

I plan to implement these functions in JIRA MESOS-3943, If you think we should 
merge them together, I am happy to do that.


- Yongqiao


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


On Dec. 8, 2015, 5:20 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40469/
> ---
> 
> (Updated Dec. 8, 2015, 5:20 a.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3956
> https://issues.apache.org/jira/browse/MESOS-3956
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update Allocator interface to support dynamic roles
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/tests/allocator.hpp 

Re: Review Request 39848: Validate revocable resources

[Guangya Liu]

> On 十二月 5, 2015, 12:21 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1503
> > 
> >
> > I think at this point it's worthwhile to factor out the validations in 
> > this function into slave/validation.cpp file like we have done in 
> > master/validation.cpp. The validation should likely return an object 
> > (TaskStatus?) that contains the message and REASON to be used in the status 
> > update.
> > 
> > This would also help with unit testing the validations more easily.
> 
> Guangya Liu wrote:
> OK, will factor out the validations to slave/validation.cpp. Can you 
> please help check if the logic is OK here? As I was using one double value to 
> minus another, shall we use CHECK_NEAR as the solution in 
> https://reviews.apache.org/r/40767/diff/1#index_header ? Thanks!

Vinod, after a second think, putting the validation here may be better as the 
current validation does not fit into validate() logic in master/validation.cpp 
slave/validation.cpp well. All of the validate() APIs are returning Error, it 
may make the logic not clear if we put the above logic to slave/validation.cpp. 
Actually the current _runTask also have some validateion logic, such as 
checkpointedResources, checkpointedExecutorResources etc. What do you say? 
Thanks.


- Guangya


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


On 十一月 5, 2015, 7:36 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated 十一月 5, 2015, 7:36 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41187, 41188]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 3:36 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 10, 2015, 3:36 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 6a7a14ce8f1b9e0a85bc88bcf32bd80ea56bb7be 
>   src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 
>   src/slave/http.cpp cef568d77da430b96e8f97487a8d0406dc0a0116 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 40431: Move RoleInfo message out of allocator.proto

[Yong Qiao Wang]

> On Dec. 10, 2015, 10:40 a.m., Adam B wrote:
> > I'm not sure exactly why you needed to move RoleInfo out of 
> > allocator.proto. The RoleInfo we use for the allocator module API doesn't 
> > need to be (and perhaps shouldn't be) the same class that we use to display 
> > role information like weights in the HTTP endpoint, or even the same class 
> > that we persist in the registry. There may be some future role metadata we 
> > want to set in the HTTP endpoint that doesn't need to be passed on to the 
> > allocator (e.g. Role.description), and maybe even something we set that 
> > doesn't need to be persisted. I don't actually think we need 3 or 4 
> > separate RoleInfo-like protobufs, but I want us to think about how each of 
> > these APIs could grow apart in their notion of "important role metadata". 
> > Is there a real need to move the RoleInfo protobuf?

Currently, RoleInfo protobuf never be used for serialization, so I think we can 
remove it from allocator.proto, and define a struct in mesos.hpp to communicate 
between the allocator and master. Then for role information display, then 
current serialization way(call modle(role*) in http.cpp) is not better, and we 
should get RoleInfo protobuf back for serialization. Refer to other 
components(such as quota), I propose to define role protobuf in a separated 
package rather than define it in mesos.proto. And for persist, we should define 
proto message in registry.proto which only contians the metadata should be 
persisted.


- Yong Qiao


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


On Dec. 8, 2015, 5:20 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40431/
> ---
> 
> (Updated Dec. 8, 2015, 5:20 a.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3944
> https://issues.apache.org/jira/browse/MESOS-3944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently role protobuf is defined in allocator.proto due to only the 
> traditional DRF allocator uses roles as it’s first level of hierarchy, I 
> think we should move it out and define it in a separated file as quota had in 
> dynamic roles project, because role protobuf will also be used by master to 
> persist.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   include/mesos/role/role.hpp PRE-CREATION 
>   include/mesos/role/role.proto PRE-CREATION 
>   src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/40431/diff/
> 
> 
> Testing
> ---
> 
> 1. Make Check successfully;
> 
> 2. $ curl http://9.110.48.168:5050/roles
> {"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

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

(Updated Dec. 10, 2015, 5:03 p.m.)


Review request for mesos, Niklas Nielsen and Vinod Kone.


Changes
---

Nik & Vinod issues addressed (:


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


Repository: mesos


Description
---

Added Load QoS Controller for the simple eviction when system load is above 
configured system load threshold for 5min and 15min:
- Made os::loadavg called from the lambda and passed via the contructor.
- Added unit test.


Diffs (updated)
-

  src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
  src/slave/qos_controllers/load.hpp PRE-CREATION 
  src/slave/qos_controllers/load.cpp PRE-CREATION 
  src/tests/oversubscription_tests.cpp 0333281c247dd182860a49f39be791c00679bf6b 

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


Testing
---

make check
run mesos with org_apache_mesos_LoadQoSController module included.


Thanks,

Bartek Plotka

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Gru 9, 2015, 10:46 po południu, Vinod Kone wrote:
> > src/Makefile.am, line 1600
> > 
> >
> > Great to see a default QoS controller!
> 
> Bartek Plotka wrote:
> Thx, can i `fix` that? =DDD
> 
> Niklas Nielsen wrote:
> @vinod - maybe wasn't meant as an issue?

i am just kidding, sure it is (:


- Bartek


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


On Gru 10, 2015, 5:03 po południu, Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Gru 10, 2015, 5:03 po południu)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> ---
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 41113: Added `IsolatorRecoveryInfo` message as the sole parameter to `Isolator::recover()`.

[Greg Mann]

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


NOTE: this patch may not be necessary, as a simpler workaround has been found. 
I'm leaving it open for now, but may discard in the near future.

- Greg Mann


On Dec. 9, 2015, 11:59 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41113/
> ---
> 
> (Updated Dec. 9, 2015, 11:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Neil Conway.
> 
> 
> Bugs: MESOS-4003
> https://issues.apache.org/jira/browse/MESOS-4003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the message `IsolatorRecoveryInfo` and makes it the sole 
> parameter of `Isolator::recover()`, in order to facilitate the future 
> addition of parameters without breaking the interface.
> 
> In addition to the two existing parameters of `Isolator::recover()`, a third 
> member was added to `IsolatorRecoveryInfo`: the agent's `work_dir`. This is 
> useful for the network storage DVD isolator 
> (https://github.com/emccode/mesos-module-dvdi) in particular.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp 95a2933988ea7c9b9404df5e12031f134712b2b5 
>   include/mesos/slave/isolator.proto d2032adf9336119ed8e1ff3c813d657d70331b67 
>   src/common/protobuf_utils.hpp 7280c9fe36726df6b02ff468c7bd5ecedf5f5023 
>   src/common/protobuf_utils.cpp 6e1eb0b8465809d1da5dac1cd29b692b9fa6ff66 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6dad2e858b68cf47e048d49d34af4fa4cb3b6841 
>   src/slave/containerizer/mesos/isolator.hpp 
> 937f253656d36ed10b47ceeb0b6101f212e65586 
>   src/slave/containerizer/mesos/isolator.cpp 
> 493b5dd26cf0e8f986381a502cfa6d1dde6573d4 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
> 123b9ed3ccaebcd5da24fc62ff7a92d4a81ed760 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
> 3b95e195ad704f163c245175390d9a26bde7e17c 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.hpp 
> 09952369c72d3c6322ae7a1c73cd68226d452ad2 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.cpp 
> 2ddb9f4adbb879682cd39966ab974cf3fa32209c 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
> 5eaf49f1f35c93ad4465adb6c9c9cf57b3a2c6ee 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
> b7ba00bc495001380f01737e46e8671ffe1c2ef7 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> b8d47e8250a892fa333a0a966a0f38fe1f2816f2 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8442e9c30612fa04f34130b9f967cb1414880ca6 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
> c3544aa313cbb185efb03bba59961cdf2b616a37 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> 00ff84b6cd0aa29fa5a7918d7f88d480af8752ca 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.hpp 
> 2e457015a0348a457581edf493877b71fab17090 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> 361ed6561bd5e2f75d026922def01f42b43d61c2 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
> c2d1455249618f9cd2e17dc2244b184d52b32eaf 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> d65c1593b44f4b21237581147e57e441ebf3160d 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> cbb94077d46d7b87ffc09b72e02269bc16f25f92 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 89bb36f936417de8169a2442729fbd7c9d60acb7 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 7e1ebc2fada5a5e291e84c7044bdba9a71f4b42c 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> 31808c1e8199fbf2cea36c273860fdbf0a2388f8 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> d971db09083faad08f3cf18c25a79245321d1d9a 
>   src/tests/containerizer/isolator.hpp 
> e4101b188560bd857ea104f61f52f27c880e7731 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fe679354d04d68b68e168cd8c4eab23898f6532f 
> 
> Diff: https://reviews.apache.org/r/41113/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` was run on OSX and Ubuntu in order to test both the 
> posix and linux isolator code. Only expected test failures were observed. 
> Some of the failures on Ubuntu are `SlaveRecoveryTest`s, which is a bit 
> disconcerting, but this issue is tracked in MESOS-4025, and seems to be due 
> to artifacts left behind by previous tests. If I do:
> 
> `GTEST_FILTER="" make check`
> `sudo GTEST_FILTER="SlaveRecoveryTest*" bin/mesos-tests.sh`
> 
> then all the `SlaveRecoveryTest`s pass.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40582: Windows: 3/3 Enabled zlib compression tests.

[Dario Bazan]

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

(Updated Dec. 10, 2015, 6:14 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Windows: 3/3 Enabled zlib compression tests.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
9893d741cd7c611dc65eba76be03e06dac618132 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
741639a942971e48e2dac42db238d423e61cac21 
  3rdparty/libprocess/3rdparty/stout/include/stout/gzip.hpp 
85c773ac675c88b313dffda7a9c32bac42ebe62d 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/gzip.hpp 
d5abf413f7debdf349bee5f8603415095f048816 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/gzip.hpp 
017cfb336ae651c150ca35468d5c19838398b419 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
26e1377ee625748b7fdbec327fef9ac602535f83 

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


Testing
---


Thanks,

Dario Bazan

Re: Review Request 41113: Added `IsolatorRecoveryInfo` message as the sole parameter to `Isolator::recover()`.

[Greg Mann]

> On Dec. 10, 2015, 5:18 p.m., Neil Conway wrote:
> > Re: "facilating the addition of new parameters without breaking the 
> > interface", is the goal to ensure ABI compatibility or API compatibility? 
> > If the former, a protobuf is not sufficient (you typically need to use the 
> > PIML idiom or something similar).

The intention is to maintain API compatibility only - requiring recompilation 
of modules for each version of Mesos is acceptable, but our hope was to 
minimize the number of code changes that module developers need to make.


- Greg


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


On Dec. 9, 2015, 11:59 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41113/
> ---
> 
> (Updated Dec. 9, 2015, 11:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Neil Conway.
> 
> 
> Bugs: MESOS-4003
> https://issues.apache.org/jira/browse/MESOS-4003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the message `IsolatorRecoveryInfo` and makes it the sole 
> parameter of `Isolator::recover()`, in order to facilitate the future 
> addition of parameters without breaking the interface.
> 
> In addition to the two existing parameters of `Isolator::recover()`, a third 
> member was added to `IsolatorRecoveryInfo`: the agent's `work_dir`. This is 
> useful for the network storage DVD isolator 
> (https://github.com/emccode/mesos-module-dvdi) in particular.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp 95a2933988ea7c9b9404df5e12031f134712b2b5 
>   include/mesos/slave/isolator.proto d2032adf9336119ed8e1ff3c813d657d70331b67 
>   src/common/protobuf_utils.hpp 7280c9fe36726df6b02ff468c7bd5ecedf5f5023 
>   src/common/protobuf_utils.cpp 6e1eb0b8465809d1da5dac1cd29b692b9fa6ff66 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6dad2e858b68cf47e048d49d34af4fa4cb3b6841 
>   src/slave/containerizer/mesos/isolator.hpp 
> 937f253656d36ed10b47ceeb0b6101f212e65586 
>   src/slave/containerizer/mesos/isolator.cpp 
> 493b5dd26cf0e8f986381a502cfa6d1dde6573d4 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
> 123b9ed3ccaebcd5da24fc62ff7a92d4a81ed760 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
> 3b95e195ad704f163c245175390d9a26bde7e17c 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.hpp 
> 09952369c72d3c6322ae7a1c73cd68226d452ad2 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.cpp 
> 2ddb9f4adbb879682cd39966ab974cf3fa32209c 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
> 5eaf49f1f35c93ad4465adb6c9c9cf57b3a2c6ee 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
> b7ba00bc495001380f01737e46e8671ffe1c2ef7 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> b8d47e8250a892fa333a0a966a0f38fe1f2816f2 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8442e9c30612fa04f34130b9f967cb1414880ca6 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
> c3544aa313cbb185efb03bba59961cdf2b616a37 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> 00ff84b6cd0aa29fa5a7918d7f88d480af8752ca 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.hpp 
> 2e457015a0348a457581edf493877b71fab17090 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> 361ed6561bd5e2f75d026922def01f42b43d61c2 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
> c2d1455249618f9cd2e17dc2244b184d52b32eaf 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> d65c1593b44f4b21237581147e57e441ebf3160d 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> cbb94077d46d7b87ffc09b72e02269bc16f25f92 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 89bb36f936417de8169a2442729fbd7c9d60acb7 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 7e1ebc2fada5a5e291e84c7044bdba9a71f4b42c 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> 31808c1e8199fbf2cea36c273860fdbf0a2388f8 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> d971db09083faad08f3cf18c25a79245321d1d9a 
>   src/tests/containerizer/isolator.hpp 
> e4101b188560bd857ea104f61f52f27c880e7731 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fe679354d04d68b68e168cd8c4eab23898f6532f 
> 
> Diff: https://reviews.apache.org/r/41113/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` was run on OSX and Ubuntu in order to test both the 
> posix and linux isolator code. Only expected test failures were observed. 
> Some of the failures on Ubuntu are `SlaveRecoveryTest`s, which is a bit 
> 

Re: Review Request 41234: Close leaked FD in RegistryClientTest.SimpleRegistryPuller.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41234]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 11, 2015, 1:15 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41234/
> ---
> 
> (Updated Dec. 11, 2015, 1:15 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jojy Varghese, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Close leaked FD in RegistryClientTest.SimpleRegistryPuller.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 3f1717b770e139c3759aab0aeda9dbcf5029b0c2 
> 
> Diff: https://reviews.apache.org/r/41234/diff/
> 
> 
> Testing
> ---
> 
> make check 
> 
> bin/mesos-tests.sh --gtest_filter="RegistryClientTest.SimpleRegistryPuller" 
> --gtest_repeat=256
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40553: Enable mesos tests installation

[Benjamin Bannier]

> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/memory_test_helper.cpp, line 197
> > 
> >
> > Should probably be passed as an arg (`bool` or even better an `enum` 
> > value). You can always pick a default.
> 
> James Peach wrote:
> I am not sure that I follow what you mean here. Are you suggesting that 
> the caller should specify where to search, based on the 
> ```MESOS_INSTALL_TESTS``` define? If so, I don't think the callers should 
> need to know that. The default needs to be consistent across a test run, 
> which implies that specific tests should not be toggling this in an ad-hoc 
> fashion.

Agreed, let's not open up the possibility of callers making this even more 
complicated.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.hpp, line 38
> > 
> >
> > These functions cannot promise to return *valid* paths; they should 
> > probably all return `Try` instead. Please also check their impls.
> 
> James Peach wrote:
> Well they may return a valid path to something that doesn't exist, which 
> is fine and will cause the test to fail. It might be reasonable to return a 
> ```Try``` and change the callers to do ```findX().get()```, which would 
> ```ABORT``` at the call-site rather than some way into the test.

Looking at the call sites I agree that just `string`s might be good enough.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.cpp, line 56
> > 
> >
> > All the `findX` functions here (but maybe `findModuleDir`) seem very 
> > easy to use incorrectly. We might e.g., depending on what is in our build 
> > directory or in the install prefix load a mix of different versions.
> > 
> > I would much prefer if we'd either load from one place or the other.
> > 
> > Going down that path would probably also lead us to remove a lot of the 
> > duplication here, by e.g., calling once into `findModuleDir` and building 
> > up the more specialized functions from there.
> 
> James Peach wrote:
> Yes, forcing a blanket policy is exactly why I needed the ```findX``` 
> family. There are only 2 scenarios that can cause inconsistent paths to tbe 
> used: (1) if you run the tests from an installation and do not specify 
> --build_dir=/none you could run tests from installed version A with helpers 
> from built version B, and (2) if you nuke some parts of the build directory 
> before running the tests.
> 
> I avoided forcing either one path or the other because it seemed 
> reasonable to run "make check" before installing. It would be simpler to 
> always force the tests to be installed before running in the 
> ```--enable-tests-install``` case if we could agree that "make check" would 
> fail in that case.
> 
> In general, I don't see these functions as easily composable. Some of 
> them happen to produce the same path string, but there's no real relationship 
> between the launcher directory and the test helper directory, for example.

Yes, I see.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.cpp, line 64
> > 
> >
> > Here and in the following: I have the feeling explicitly passing in a 
> > flag (e.g., `bool searchInstallationDirectory`, or even clearer with an 
> > `enum` value) would make this easier to digest. All of these could have a 
> > default.
> 
> James Peach wrote:
> How would a test know whether to specify 
> ```searchInstallationDirectory```? A global command-line flag?

One could set the value of a static const bool variable in this TU depending on 
whether `MESOS_INSTALL_TESTS` was defined. The benefit would be that no matter 
whether it was there or not, any compile would check all branches in these 
functions (but very likely optimize away the dead branch).


- Benjamin


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


On Nov. 30, 2015, 6:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40553/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Bugs: MESOS-3608
> https://issues.apache.org/jira/browse/MESOS-3608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables the installation mesos-tests and its dependencies
> and helper tool. The goal is to 

Re: Review Request 41185: CMake: Updated LFLAG for dl library to defined label

[Alex Clemmer]

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



3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake (line 114)


One could argue that this flag should be defined twice, once for Stout and 
once for Process. We have done stuff like that in the past, but I think here 
it's a bit too much of a nit even for a pedant like me. :)


- Alex Clemmer


On Dec. 10, 2015, 3:26 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41185/
> ---
> 
> (Updated Dec. 10, 2015, 3:26 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Updated LFLAG for dl library to defined label
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> 9893d741cd7c611dc65eba76be03e06dac618132 
> 
> Diff: https://reviews.apache.org/r/41185/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Review Request 41243: Update how we find the .git directory in bootstrap

[Kevin Klues]

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

Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

When building from git, bootstrap will (among other things) install
pre-commit and post-rewrite hooks into the .git/hooks directory of the
mesos tree. However the current implementation always assumes that .git
exists in the same directory as the bootstrap file. This may not always
be true.

Most notably, it is not true if the mesos tree is included as a
submodule inside another project. When included as a submodule, .git is
no longer a directory, but rather a file whose text contains a pointer
back to the actual location of the .git folder inside the containing
project. To get at this directory, we need to run 'git rev-parse
--git-dir' instead of simply assuming that the local .git is the proper
directory.


Diffs
-

  bootstrap 89d986fd95dc16bbb79623ef92e3b14a2e7009f9 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 41158: Turn off rx checksum offloading for veth in container

[Cong Wang]

> On Dec. 11, 2015, 1:16 a.m., David Robinson wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp, lines 
> > 1097-1098
> > 
> >
> > This writes to stderr, which can end up in the logs.
> > 
> > [root@server ~]# ethtool --version 1> /dev/null
> > ethtool version 6
> > Usage:
> > ethtool DEVNAME Display standard information about device
> > 
> > 
> > Log snippet:
> > 
> > I1211 01:05:13.215730 10885 main.cpp:190] Build: 2015-12-10 22:54:33 by 
> > mockbuild
> > I1211 01:05:13.215859 10885 main.cpp:192] Version: 0.26.0-tw5
> > I1211 01:05:13.215996 10885 containerizer.cpp:142] Using isolation: 
> > cgroups/cpu,cgroups/mem,network/port_mapping,posix/disk,cgroups/perf_event,filesystem/posix
> > ethtool version 6
> > Usage:
> > ethtool DEVNAME Display standard information about device
> > I1211 01:05:13.251729 10885 port_mapping.cpp:1255] Using eth0 as the 
> > public interface
> > I1211 01:05:13.252707 10885 port_mapping.cpp:1280] Using lo as the 
> > loopback interface

Ah, yet another difference on Fedora...

$ ethtool --version | grep v
ethtool version 3.8

Let me fix it.


- Cong


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


On Dec. 9, 2015, 11:15 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41158/
> ---
> 
> (Updated Dec. 9, 2015, 11:15 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Bugs: MESOS-4105
> https://issues.apache.org/jira/browse/MESOS-4105
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We noticed that in some cases we delivered some corrupt packets to 
> applications running in our containers. This is clearly wrong. 
> 
> Here is what happens:
> 
> 1) We receive a corrupt packet externally
> 2) The hardware driver is able to checksum it and notices it has a bad 
> checksum
> 3) The driver delivers this packet anyway to wait for TCP layer to checksum 
> it again and then drop it
> 4) This packet is moved to a veth interface because it is for a container
> 5) Both sides of the veth pair have RX checksum offloading by default
> 6) The veth_xmit() marks the packet's checksum as UNNECESSARY since its peer 
> device has rx checksum offloading
> 7) Packet is moved into the container TCP/IP stack
> 8) TCP layer is not going to checksum it since it is not necessary
> 9) The packet gets delivered to application layer
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 89bb36f936417de8169a2442729fbd7c9d60acb7 
> 
> Diff: https://reviews.apache.org/r/41158/diff/
> 
> 
> Testing
> ---
> 
> 1) Turn rx checksum off manually and the bug is gone
> 2) Test this patch and verify rx checksum is turned off as expected.
> 3) I don't see any noticable performance issue after turning this off
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Review Request 41245: Use ethtool -k lo to check ethtool command

[Cong Wang]

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

Review request for mesos, David Robinson, Ian Downes, and Jie Yu.


Repository: mesos


Description
---

David reported that on CentOS5 part of ethtool --version output sends to stderr 
instead of stdout, which leads this output in our log.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
7c1724a4ca5c45bb214e5129743e0644c9ca9661 

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


Testing
---

Manually start mesos slave


Thanks,

Cong Wang

Re: Review Request 40906: Replaced `BadRequest` with `MethodNotAllowed` for all HTTP requests with unsupported methods.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40905, 40913, 40906]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 12:13 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40906/
> ---
> 
> (Updated Dec. 10, 2015, 12:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Isabel Jimenez, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4056
> https://issues.apache.org/jira/browse/MESOS-4056
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
> 
> Diff: https://reviews.apache.org/r/40906/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/process.hpp, lines 65-86
> > 
> >
> > Why isn't this in the http header? It looks like firewall belongs there 
> > too since it's more aptly namespaced as 'http::Firewall'?

I will move the firewall functions in a different request.


> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1205-1223
> > 
> >
> > Can you instead just add another handler to the existing HttpProcess 
> > above, rather than introducing another Process here?
> > 
> > There is already a handler for 'auth', so we can add one called 
> > '/authenticated' and document why '/auth' is still there (looks like it is 
> > going to be obviated by this?).

About this one, there is one test that checks authentication which is handled 
through the auth method. That test (and its functionality) become obsolete with 
this patch, should I just removed?


> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2400-2402
> > 
> >
> > Why are you dispatching here rather than calling directly? Note though 
> > that I'm not sure we should keep `schedule`.

Mostly because BenH has strongly discourage me from calling methods directly 
from processes. Moreover, once the cleaning of unused sequences is implemented, 
is imposible to guarantee that requests are being done from the same thread 
always, so we may have concurrency issues.


- Alexander


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


On Dec. 8, 2015, 3:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Dec. 8, 2015, 3:38 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/authentication_router.hpp 
> 5777deafd7420324627802a7ab9da9aaa2b46825 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

> On Dec. 9, 2015, 12:19 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1339-1343
> > 
> >
> > Hm.. it looks like you're using the principals here to check that the 
> > requests arrive to the endpoint in the right order?
> > 
> > How about avoiding the captures here and just specifying "principal1" 
> > and "principal2" in the expectation?
> > 
> > Also, you're returning "1" and "2" but that's not checked at all? Did 
> > you mean to check that the responses below are ordered correctly?

It is not enough to just set the principal in the expectation, since the 
expectation only says that the function will be called with the given 
parameters, but mentions nothing about the ordering.


> On Dec. 9, 2015, 12:19 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1373-1375
> > 
> >
> > It's not clear why the sleep is here. If I had to guess, it looks like 
> > you wanted to ensure that the both authentication calls were made at this 
> > point? If so, we don't need a sleep for this.
> > 
> > Let's try to avoid the sleep here if possible.

When pipelining is not enforced, without the sleep the test is just flaky, 
since often the messages will be executed in the right order. I can remove it, 
but as I mentioned, the test passes from failing everytime to only sometimes.


- Alexander


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


On Dec. 8, 2015, 3:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Dec. 8, 2015, 3:38 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/authentication_router.hpp 
> 5777deafd7420324627802a7ab9da9aaa2b46825 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 40532: Added notion of evictable task to RunTaskMessage.

[Guangya Liu]

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

(Updated 十二月 10, 2015, 2:26 p.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
and Klaus Ma.


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


Repository: mesos


Description
---

Added notion of evictable task to RunTaskMessage.


Diffs (updated)
-

  src/messages/messages.proto b66bac14056407140bb3cf5aa3e67ac94df1a2f8 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 38950: Http Authenticators can be loaded as modules from mesos.

[Alexander Rojas]

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

(Updated Dec. 10, 2015, 3:32 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Adds support for modularization of HTTP Authenticators. 

It includes an example of how to do it with the Basic HTTP Authenticator.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
PRE-CREATION 
  include/mesos/module/http_authenticator.hpp PRE-CREATION 
  src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb 
  src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  src/authentication/http/basic_authenticator_factory.cpp PRE-CREATION 
  src/examples/test_http_authenticator_module.cpp PRE-CREATION 
  src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
  src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
  src/module/manager.cpp 1f04790510a2ab9ccd6907fd01be192f52ee90c6 
  src/tests/http_authentication_tests.cpp PRE-CREATION 
  src/tests/module.hpp e46ed12c80707bf44ceef3ed1a4eb2f321ce10f6 
  src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 10, 2015, 3:25 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Changes after ben's review.


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


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/http.hpp 
3f33b46527e07ed73621e07e9c5d40594e32f6a9 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/authentication_router.hpp 
5777deafd7420324627802a7ab9da9aaa2b46825 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38094: Added implementation of Http Basic authentication scheme.

[Alexander Rojas]

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

(Updated Dec. 10, 2015, 3:28 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Rebase changes in previous review.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 82778bbd34cfcf54b4268e019d634eb64e506d55 
  3rdparty/libprocess/include/process/authenticator.hpp 
5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f 
  3rdparty/libprocess/src/CMakeLists.txt 
681f0cfec57e152568da41698c8bdd52c05f65a6 
  3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rojas]

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

(Updated Dec. 10, 2015, 3:34 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
Toenshoff.


Changes
---

Updates for changes in previous reviews.


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


Repository: mesos


Description
---

1. Adds a flag to load an HTTP Authenticator module from the flags.
2. If provided, uses the credentials file to initialize the default HTTP 
Authenticator.
3. Updates the existing endpoints which implement their own basic HTTP 
authenticator with the libprocess one.
4. Updates one test which expected the wrong results since now credentials are 
checked before the body of the request.


Diffs (updated)
-

  src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
  src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
  src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
  src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
  src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161 
  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38950: Http Authenticators can be loaded as modules from mesos.

[Till Toenshoff]

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



src/examples/test_http_authenticator_module.cpp (lines 49 - 50)


s/a/an/
s/Authorizer/http authenticator/



src/master/constants.hpp (line 131)


s/Basic/basic/ ?



src/tests/module.cpp (line 216)


s/authorizer/authenticator/


- Till Toenshoff


On Dec. 10, 2015, 2:32 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38950/
> ---
> 
> (Updated Dec. 10, 2015, 2:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support for modularization of HTTP Authenticators. 
> 
> It includes an example of how to do it with the Basic HTTP Authenticator.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> PRE-CREATION 
>   include/mesos/module/http_authenticator.hpp PRE-CREATION 
>   src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb 
>   src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   src/authentication/http/basic_authenticator_factory.cpp PRE-CREATION 
>   src/examples/test_http_authenticator_module.cpp PRE-CREATION 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
>   src/module/manager.cpp 1f04790510a2ab9ccd6907fd01be192f52ee90c6 
>   src/tests/http_authentication_tests.cpp PRE-CREATION 
>   src/tests/module.hpp e46ed12c80707bf44ceef3ed1a4eb2f321ce10f6 
>   src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 
> 
> Diff: https://reviews.apache.org/r/38950/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 40553: Enable mesos tests installation

[Till Toenshoff]

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


Great work James, much appreciated.

And yes, it is tedious and hence the results appear unclean at the first glance 
here and there. I did play around a bit with the introduced helpers / getters, 
trying to streamline them but in the end could not come with something that had 
more beauty than your drafted solution.

There are a few  style nits in here, but I refrained from commenting on those 
in this early stage.


src/tests/utils.cpp (lines 71 - 77)


I still don't understand why the oversubscription tests need to have this 
installed in the LIBDIR and not in MODULE_DIR folder. We need to find some 
proper reasoning here before making so much extra fuzz for this, I feel.



configure.ac (line 330)


Could you please explain the [1] for the negative case?



src/tests/containerizer/memory_test_helper.cpp (line 187)


Can you explain briefly what you mean here by "too many dependencies" - 
seemed to me we only "drag" in "stout/json.hpp"?!



src/tests/containerizer/memory_test_helper.cpp (line 188)


`getTestHelper` ?



src/tests/module_tests.cpp (lines 60 - 68)


Seems this could be covered by `getModulePath`, no?


- Till Toenshoff


On Nov. 30, 2015, 6:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40553/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Bugs: MESOS-3608
> https://issues.apache.org/jira/browse/MESOS-3608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables the installation mesos-tests and its dependencies
> and helper tool. The goal is to allow operators to build a separate
> test package that can be run at deployment time to verify that Mesos
> works in the deployment environment.
> 
> Since the build directory is searched first, to run it on a host
> that has a build tree, you need to specify a non-existent tree:
> 
> ~ $ $PREFIX/libexec/mesos/tests/mesos-tests --build_dir=/none
> 
> - Add --enable-tests-install
> - Fix mesos-tests gmock dependencies
> - Optionally install tests, helpers and test modules
> - Add utility helpers to find various test resources
> 
> Current test status:
> 
>   [==] 784 tests from 106 test cases ran. (135304 ms total)
>   [  PASSED  ] 780 tests.
>   [  FAILED  ] 4 tests, listed below:
>   [  FAILED  ] ExamplesTest.TestFramework
>   [  FAILED  ] ExamplesTest.NoExecutorFramework
>   [  FAILED  ] ExamplesTest.EventCallFramework
>   [  FAILED  ] ExamplesTest.PersistentVolumeFramework
> 
> 
> Diffs
> -
> 
>   configure.ac b30a8d30076f3068fd7d5fc8ccea1982639e998a 
>   src/Makefile.am fd38cfa73d81a98c819378f99a766e2ddb7e1a04 
>   src/tests/containerizer/launch_tests.cpp 
> c7ebe2606e4ff99ced90342dd16e0b4bf02bc504 
>   src/tests/containerizer/memory_test_helper.cpp 
> 4a3de2e3c887aa6afc604588850e1386f92d8c11 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fe679354d04d68b68e168cd8c4eab23898f6532f 
>   src/tests/containerizer/ns_tests.cpp 
> 603e54b7303c5aa15e2c5715dc7a2f7e7d39541b 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 
>   src/tests/environment.cpp 36f0ad0b739559e5c883d72585731944da4283cf 
>   src/tests/fetcher_tests.cpp 99c494a2167ea9ec94e63d92980df17a4089f95a 
>   src/tests/health_check_tests.cpp b1454b085b36bb7c4d8ef012c764cd8466b4fb02 
>   src/tests/mesos.cpp a75182ec7f6d0c6063bbb36a6ccf4e8a4a81c8dd 
>   src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 
>   src/tests/module_tests.cpp b5cfbcd4885a4f2c20b19e00958fedda81afa6e0 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
>   src/tests/script.cpp ee44fef29fb40e414d7507168091ee5cd0d15736 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
>   src/tests/utils.hpp a6cca472f4dfab12cd6eccab6972206d842177aa 
>   src/tests/utils.cpp 877139e97249761658dce3b1058cdc2e2a52367b 
> 
> Diff: https://reviews.apache.org/r/40553/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 40872: Changed RegistryClientTest to use instance work directory.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Dec. 2, 2015, 4:23 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40872/
> ---
> 
> (Updated Dec. 2, 2015, 4:23 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClientTest suite uses static variable for is work directory. This has
> been changed to model the TemporaryDirectoryTest suite.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40872/diff/
> 
> 
> Testing
> ---
> 
> make check (600 times).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41096: CMake: Added LFLAGs need for linux cmake build

[Alex Clemmer]

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



3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake (line 97)


So the `curl` flag was here for Windows but not for Linux.

Does this work for you without this flag, by the way? Just curious. We 
should still add it (and in fact, I should have added it some time ago).


- Alex Clemmer


On Dec. 10, 2015, 3:16 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41096/
> ---
> 
> (Updated Dec. 10, 2015, 3:16 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> a9cd9902567cef4c7ab4125463a68e19907db0b4 
> 
> Diff: https://reviews.apache.org/r/41096/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 40905: [libprocess]: Made `MethodNotAllowed` response compliant to RFC 2616.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 10, 2015, 12:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40905/
> ---
> 
> (Updated Dec. 10, 2015, 12:12 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Isabel Jimenez, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4056
> https://issues.apache.org/jira/browse/MESOS-4056
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According to RFC 2616 
> (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.7), 
> `MethodNotAllowed` response must include an 'Allow' header containing a list 
> of valid methods.
> 
> This is a libprocess change only.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 3f33b46527e07ed73621e07e9c5d40594e32f6a9 
> 
> Diff: https://reviews.apache.org/r/40905/diff/
> 
> 
> Testing
> ---
> 
> This a libprocess change only. The code does not compile without the 
> follow-up [mesos] patch https://reviews.apache.org/r/40913/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40913: Made `MethodNotAllowed` response compliant to RFC 2616.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 10, 2015, 12:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40913/
> ---
> 
> (Updated Dec. 10, 2015, 12:12 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Isabel Jimenez, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4056
> https://issues.apache.org/jira/browse/MESOS-4056
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According to RFC 2616 
> (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.7), 
> `MethodNotAllowed` response must include an 'Allow' header containing a list 
> of valid methods.
> 
> This updates `MethodNotAllowed` c-tor invocations in Mesos codebase.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/slave/http.cpp cef568d77da430b96e8f97487a8d0406dc0a0116 
>   src/tests/executor_http_api_tests.cpp 
> 1be657c64d9ead14f1e2b41e35fd6ca04a0a56a4 
>   src/tests/scheduler_http_api_tests.cpp 
> 4f52309ff50a5b56cf20f2c5cfddd9c10b2b75d9 
> 
> Diff: https://reviews.apache.org/r/40913/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>