Re: Review Request 34436: Factor out launch helper for easier reuse
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34436/#review84884
---
Where will this get re-used?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34436/#comment136329
It doesn't use the MesosContainerizer, it uses a helper that the MC uses.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34436/#comment136325
Why this change? This function shouldn't take a Try.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34436/#comment136330
{.., ..}?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34436/#comment136327
s/we/We/
s/clutter/noise
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34436/#comment136328
How much noise is there? This seems onerous to users to modify source and
recompile to enable some debug output...
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34436/#comment136326
This should be done by the caller when the launcher is created, indeed it's
done with a number of CHECK_SOMEs. What's the motivation to changing this
function?
- Ian Downes
On May 21, 2015, 4:31 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34436/
---
(Updated May 21, 2015, 4:31 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2332
https://issues.apache.org/jira/browse/MESOS-2332
Repository: mesos
Description
---
Factor out launch helper for easier reuse
Diffs
-
src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502
Diff: https://reviews.apache.org/r/34436/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/#review84882 --- Ship it! src/master/master.cpp https://reviews.apache.org/r/33159/#comment136310 I don't think this comment adds any value since the following line is pretty self explanatory (though the comment 4 lines before suffers of the same ailment) - Alexander Rojas On April 20, 2015, 8:46 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated April 20, 2015, 8:46 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs - src/master/allocator/allocator.hpp 91f80501aa9bc733fd53f9cb1ac0f15949a74964 src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34432: Remove duplicate constant string references to mesos-containerizer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34432/#review84885 --- src/slave/containerizer/isolators/network/port_mapping.hpp https://reviews.apache.org/r/34432/#comment136331 Please state why they are exposed. src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/34432/#comment136332 Why not be consistent with the other const string inlines? - Ian Downes On May 21, 2015, 4:32 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34432/ --- (Updated May 21, 2015, 4:32 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2754 https://issues.apache.org/jira/browse/MESOS-2754 Repository: mesos Description --- Remove duplicate constant string references to mesos-containerizer, net_tcp_rtt_... Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/34432/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34558: Move port mapping isolator configuration settings to file local scope for easier sharing.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34558/#review84881
---
bunch of grammatical stuff, much of it not actually yours but might as well
clean it up while you're in this file.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136309
any particular reason why? what about 1/1024/1024?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136307
Don't include the ';' in these strings.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136311
Below the start of ip_local_port_range?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136312
What's the implication of a conflict? Does the test fail on conflict?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136308
Ditto and elsewhere.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136313
s/tracking/track/
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136314
s/persistant/persistent/
specifically, the persistentPorts range defined above.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136315
s/defined/define/
s/container 1/container1/
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136316
s/ - /; /
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136317
s/connection/connecting/
can they even make a connection?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136318
drop or pull this comment up
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136319
strings::join(;, {...})?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136320
s/2/two/
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136321
sucessful in that data is written to the appropriate file?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136322
s/get/receive/
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/34558/#comment136324
s/a nc/an nc/ reads better, I think.
ditto elsewhere
- Ian Downes
On May 21, 2015, 4:31 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34558/
---
(Updated May 21, 2015, 4:31 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2332
https://issues.apache.org/jira/browse/MESOS-2332
Repository: mesos
Description
---
Move port mapping isolator configuration settings to file local scope for
easier sharing.
Diffs
-
src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502
Diff: https://reviews.apache.org/r/34558/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 33159: Pump updateFramework through Allocator from Master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/#review84893 --- src/master/allocator/mesos/hierarchical.hpp https://reviews.apache.org/r/33159/#comment136338 According to this comment, I assume that now we do not allow frameworks to re-register with a new role or checkpoint flag. If this is the case, does it make sense to verify and therefore document these assumptions? Something like ``` CHECK(frameworks[frameworkId].role == frameworkInfo.role()) ``` Without this, the patch looks like harness code for future changes. On the same page, we do allow some of the `FrameworkInfo`'s fields to change on the re-registration. IIRC, they are all irrelevant to allocation, but do you mind throwing a comment about that? - Alexander Rukletsov On April 20, 2015, 6:46 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33159/ --- (Updated April 20, 2015, 6:46 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-2615 and MESOS-703 https://issues.apache.org/jira/browse/MESOS-2615 https://issues.apache.org/jira/browse/MESOS-703 Repository: mesos Description --- Follow up of 32961 where we add the 'updateFramework' function to the allocator. Diffs - src/master/allocator/allocator.hpp 91f80501aa9bc733fd53f9cb1ac0f15949a74964 src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 Diff: https://reviews.apache.org/r/33159/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34140: Appc image store
On May 18, 2015, 4:38 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc/store.hpp, line 116 https://reviews.apache.org/r/34140/diff/1/?file=957276#file957276line116 Looks like this is a global store for all images. Would it make sense to make sure at most one StoreProcess can be instantiated? There should only be one Store for a given store directory, not necessarily globally. I don't think it's necessary to complicate the code to enforce uniqueness. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review84233 --- On May 12, 2015, 5:48 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 12, 2015, 5:48 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Review Request 34616: Updated allocator to properly handle oversubscribed resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34616/ --- Review request for mesos, Jie Yu and Niklas Nielsen. Bugs: MESOS-2734 https://issues.apache.org/jira/browse/MESOS-2734 Repository: mesos Description --- Just ending up resuing the existing variables (Slave.total and Slave.available) to store oversubscribed resources. The nice thing is that the changes are minimal. Also, we could potentially reuse updateSlave() to also update slave's total resources in the future. Diffs - src/master/allocator/mesos/hierarchical.hpp 4b36d42b0c4614493562e57c5eac90c6c38ca087 src/tests/hierarchical_allocator_tests.cpp 1a43dc72e739f3c55787716d680faa42a7d0d86f Diff: https://reviews.apache.org/r/34616/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34268: stout library - adding support for Solaris
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review84974 --- Patch looks great! Reviews applied: [34268] All tests passed. - Mesos ReviewBot On May 22, 2015, 7:15 p.m., Stan Teresen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 22, 2015, 7:15 p.m.) Review request for mesos, Joris Van Remoortere and Till Toenshoff. Repository: mesos-incubating Description --- stout library - adding support for Solaris Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 86949ec 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp b0ed5cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 81d64cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34268/diff/ Testing --- File Attachments adding missing new file: stout/os/sunos.hpp https://reviews.apache.org/media/uploaded/files/2015/05/15/a2e296fa-e251-4467-9873-77d8ced7f0a3__sunos.hpp Thanks, Stan Teresen
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
On May 22, 2015, 8:50 p.m., Marco Massenzio wrote: include/mesos/slave/oversubscription.proto, line 49 https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line49 I have some concerns about this design - given the Note above, this would imply that we would have multiple fields, each with its own message type (eg, `Freeze`, `Resize`, etc. etc.) Can't we just define some sort of base `ActionInfo` type, which may be extensible (maybe, even have an `ExtraInfo`). Been a while since I last played with protobuf at Google, but my concern is the potential growth of fields/types that this approach seem to entail. This is a union pattern we used consistently in the code base. For example, the new API (include/mesos/scheduler/scheduler.proto), Offer.Operation, etc. I think this is more explicit (thus more readable IMO) comparing to a more general ActionInfo type. What do you think? - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/#review84988 --- On May 22, 2015, 7:46 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 22, 2015, 7:46 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34613: Added and implemented 'update()' API call to Sorter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34613/#review85006 --- Ship it! src/master/allocator/sorter/drf/sorter.cpp https://reviews.apache.org/r/34613/#comment136485 Could you please add this CHECK to 'remove' as well just to be consistent. src/tests/sorter_tests.cpp https://reviews.apache.org/r/34613/#comment136486 s/Update/UpdateSlaveTotal/? - Jie Yu On May 22, 2015, 9:25 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34613/ --- (Updated May 22, 2015, 9:25 p.m.) Review request for mesos, Jie Yu and Niklas Nielsen. Bugs: MESOS-2729 https://issues.apache.org/jira/browse/MESOS-2729 Repository: mesos Description --- Need this for the subsequent review. Diffs - src/master/allocator/sorter/drf/sorter.hpp fd00c8ccd6ac352113ae5b2d69b2d355ba32cc8d src/master/allocator/sorter/drf/sorter.cpp f53a934b2964f9da70b1f5624916373f1f82bbec src/master/allocator/sorter/sorter.hpp f06b9462f206168999318363fc46784c2e41b19c src/tests/sorter_tests.cpp 6b0389bb17293d79ccb821c619127935a77c96cc Diff: https://reviews.apache.org/r/34613/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 22, 2015, 9:45 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- 1) FIXED Marco Massenzio: nit: s/correction/corrective action also, prefer needs to be taken 2) FIXED Marco Massenzio: not sure whether this is an artifact of RB, but your tabs seem to be out of line? Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs (updated) - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/#review84984 --- Patch looks great! Reviews applied: [34581] All tests passed. - Mesos ReviewBot On May 22, 2015, 7:46 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 22, 2015, 7:46 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34436: Factor out launch helper for easier reuse
On May 22, 2015, 7:27 a.m., Ian Downes wrote: src/tests/port_mapping_tests.cpp, lines 186-187 https://reviews.apache.org/r/34436/diff/1/?file=966863#file966863line186 How much noise is there? This seems onerous to users to modify source and recompile to enable some debug output... Output goes down from around 160 lines per test to 2 lines. The advantage of removing the output is that you can see the test progress. The information that is output to the screen by default has so far been insufficient to a aid in debugging. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34436/#review84884 --- On May 21, 2015, 11:31 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34436/ --- (Updated May 21, 2015, 11:31 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2332 https://issues.apache.org/jira/browse/MESOS-2332 Repository: mesos Description --- Factor out launch helper for easier reuse Diffs - src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/34436/diff/ Testing --- make check Thanks, Paul Brett
Review Request 34614: Added 'updateSlave()' API call to Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34614/ --- Review request for mesos, Jie Yu and Niklas Nielsen. Bugs: MESOS-2730 https://issues.apache.org/jira/browse/MESOS-2730 Repository: mesos Description --- Added an API call to update oversubscribed resources. This is a stub. Actual implementation is in a subsequent review. Diffs - include/mesos/master/allocator.hpp 0328ae4b2a6b39cefff3e64136f209f1593808cd src/master/allocator/mesos/allocator.hpp e6f611435969730baedf55ca475e5fa18d700b64 src/master/allocator/mesos/hierarchical.hpp 4b36d42b0c4614493562e57c5eac90c6c38ca087 src/tests/mesos.hpp 924b0ff7d307ce9650b78d1c698082c0cf4bb56c Diff: https://reviews.apache.org/r/34614/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34614: Added 'updateSlave()' API call to Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34614/ --- (Updated May 22, 2015, 9:42 p.m.) Review request for mesos, Jie Yu and Niklas Nielsen. Changes --- added 'depends' on. Bugs: MESOS-2730 https://issues.apache.org/jira/browse/MESOS-2730 Repository: mesos Description --- Just the stub API. Implementation is in the subsequent review. Diffs - include/mesos/master/allocator.hpp 0328ae4b2a6b39cefff3e64136f209f1593808cd src/master/allocator/mesos/allocator.hpp e6f611435969730baedf55ca475e5fa18d700b64 src/master/allocator/mesos/hierarchical.hpp 4b36d42b0c4614493562e57c5eac90c6c38ca087 src/tests/mesos.hpp 924b0ff7d307ce9650b78d1c698082c0cf4bb56c Diff: https://reviews.apache.org/r/34614/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 31505: Add flow classifiers for fq_codel on egress
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31505/#review84998
---
Two high level comments:
1) We need a slave flag to control if turning on this feature or not
2) Do you think we should at least add some unit test for this? For example,
verify that the filters are properly set up?
src/linux/routing/filter/filter.hpp
https://reviews.apache.org/r/31505/#comment136460
These two can be merged since `classid` is already Optional. So please
remove the first constructor and change the callsites accordingly.
src/linux/routing/filter/filter.hpp
https://reviews.apache.org/r/31505/#comment136461
These two can be merged too. Remove the latter and updates the callsites.
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136470
Please add a NOTE: prefix here for the comments.
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136465
We should call it flowIds:
```
flowIds[sourcePorts.get()] = classid.get().secondary();
```
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136468
s/classid/flowId/
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136466
'classid' here could be None, right? It'll cause classid.get() to crash. So
please move it to the end of this function:
```
if (flowId.isSome()) {
info-flowId = flowId.get();
freeFlowIds.erase(flowId.get());
}
```
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136469
Kill this blank line.
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136476
Rest of the host packets...
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136480
Could you please move this right above `setstring targets` and add a LOG
as well?
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136482
s/Host/host/
We use camelCase for varibles. Please fix it here and everywhere else.
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/31505/#comment136481
Please add a blank line above.
- Jie Yu
On May 17, 2015, 12:27 a.m., Cong Wang wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31505/
---
(Updated May 17, 2015, 12:27 a.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
Bugs: MESOS-2422
https://issues.apache.org/jira/browse/MESOS-2422
Repository: mesos
Description
---
Currently we do nothing on the host egress side. By default, kernel uses its
own hash function to classify the packets to different TX queues (if the
hardware interface supports multiqueue). So packets coming out of different
containers could end up queueing in the same TX queue, in this case we saw
buffer bloat on some TX queue caused packet drops.
We need to isolation the egress traffic so that containers will not have
interference with each other. The number of hardware TX queues is limited by
hardware interface, usually not enough to map our container in 1:1 way,
therefore we need some software solution. We choose fq_codel and use tc
filters to classify packets coming out of different containers to different
fq_codel flows, and the codel algorithm on each flow could also help us to
reduce the buffer bloat. Note when the packets leave fq_codel, they still
share the physical TX queue(s), this is however (almost) beyond what we can
control, we have to rely on the kernel behavior.
TODO: get some performance numbers
Diffs
-
src/linux/routing/filter/filter.hpp
024582cf8274da2e5b4ebd00fcf83d6930e2
src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76
src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa
src/slave/containerizer/isolators/network/port_mapping.hpp
c72fb47f60f40cda8d84a10497b9133f83cf018e
src/slave/containerizer/isolators/network/port_mapping.cpp
a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
Diff: https://reviews.apache.org/r/31505/diff/
Testing
---
Manually start two mesos containers with netperf running side.
Thanks,
Cong Wang
Re: Review Request 34616: Updated allocator to properly handle oversubscribed resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34616/#review85004 --- Patch looks great! Reviews applied: [34613, 34614, 34616] All tests passed. - Mesos ReviewBot On May 22, 2015, 9:41 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34616/ --- (Updated May 22, 2015, 9:41 p.m.) Review request for mesos, Jie Yu and Niklas Nielsen. Bugs: MESOS-2734 https://issues.apache.org/jira/browse/MESOS-2734 Repository: mesos Description --- Just ending up resuing the existing variables (Slave.total and Slave.available) to store oversubscribed resources. The nice thing is that the changes are minimal. Also, we could potentially reuse updateSlave() to also update slave's total resources in the future. Diffs - src/master/allocator/mesos/hierarchical.hpp 4b36d42b0c4614493562e57c5eac90c6c38ca087 src/tests/hierarchical_allocator_tests.cpp 1a43dc72e739f3c55787716d680faa42a7d0d86f Diff: https://reviews.apache.org/r/34616/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 22, 2015, 7:46 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Addressed all comments Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs (updated) - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34436: Factor out launch helper for easier reuse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34436/ --- (Updated May 22, 2015, 8:48 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Address review comments Bugs: MESOS-2332 https://issues.apache.org/jira/browse/MESOS-2332 Repository: mesos Description --- Factor out launch helper for easier reuse Diffs (updated) - src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/34436/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
include/mesos/slave/oversubscription.proto, line 30
https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line30
nit: s/correction/corrective action
also, prefer needs to be taken
Agree.
Also there is gramma mistake in my comment - s/corrections/correction
On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
include/mesos/slave/oversubscription.proto, line 42
https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line42
can you define this instead as:
```
message ActionInfo {
optional ExecutorID executor_id = 1;
optional SlaveID slave_id = 2;
optional TaskID task_id = 3;
}
```
or something similar, that makes it more generally applicable?
Hey, have you seen the Jie Yu comment in https://reviews.apache.org/r/34571/?
That previous request was as an initial work for this issue - please see.
Initially we wanted to do it in more generic way.
I partly agree with Jie - Offer.Operation is done like that.
Aslo notice that SlaveID is not needed here - the corrections are made in Slave
scope.
Additionaly, we don't want to add unnecessary fields like TaskID for now - if
we implement such functionality (killing tasks), then we will add such field
- Bartek
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
---
On May 22, 2015, 7:46 p.m., Bartek Plotka wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
---
(Updated May 22, 2015, 7:46 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod
Kone.
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
---
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs
-
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/34581/diff/
Testing
---
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in
the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/#review85016 --- Patch looks great! Reviews applied: [34581] All tests passed. - Mesos ReviewBot On May 22, 2015, 9:45 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 22, 2015, 9:45 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 13709: Fixed typos and added .gitignore
On Aug. 22, 2013, 11:38 p.m., Ben Mahler wrote: I'm ok with adding these kinds of things to .gitignore. I'm confused here - see comments to https://reviews.apache.org/r/33448 Also, what's the relationship with .gitignore-template? are we dropping it? To be clear, I fully support these changes (I was the one proposing them :) but would like to understand what's changed and how do we manage .gitignore and .gitignore-template. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13709/#review25439 --- On Aug. 22, 2013, 7:06 a.m., Kevin Lyda wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13709/ --- (Updated Aug. 22, 2013, 7:06 a.m.) Review request for mesos. Bugs: MESOS-654 https://issues.apache.org/jira/browse/MESOS-654 Repository: mesos Description --- Fixed a few typos, added a .gitignore, removed a generated file. Diffs - .gitignore PRE-CREATION 3rdparty/libprocess/install-sh 6781b98 README e94531d src/common/values.cpp ce26119 src/linux/cgroups.cpp b97a89c src/log/coordinator.cpp 6e6466f src/log/log.cpp aea06e7 src/master/master.cpp d53b8bb src/slave/slave.cpp 92a0a7e src/tests/allocator_tests.cpp c57da6e src/tests/resources_tests.cpp 964a1b6 src/tests/status_update_manager_tests.cpp cf420e4 Diff: https://reviews.apache.org/r/13709/diff/ Testing --- git clean -fxd ./bootstrap cd build ../configure make make check All these work as they did before. git status is now clean when the tree is built (unless there have been code changes made). git diff only shows code changes, not stuff from generated files. Thanks, Kevin Lyda
Re: Review Request 34614: Added 'updateSlave()' API call to Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34614/ --- (Updated May 22, 2015, 9:40 p.m.) Review request for mesos, Jie Yu and Niklas Nielsen. Bugs: MESOS-2730 https://issues.apache.org/jira/browse/MESOS-2730 Repository: mesos Description (updated) --- Just the stub API. Implementation is in the subsequent review. Diffs - include/mesos/master/allocator.hpp 0328ae4b2a6b39cefff3e64136f209f1593808cd src/master/allocator/mesos/allocator.hpp e6f611435969730baedf55ca475e5fa18d700b64 src/master/allocator/mesos/hierarchical.hpp 4b36d42b0c4614493562e57c5eac90c6c38ca087 src/tests/mesos.hpp 924b0ff7d307ce9650b78d1c698082c0cf4bb56c Diff: https://reviews.apache.org/r/34614/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34614: Added 'updateSlave()' API call to Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34614/#review85029 --- Ship it! include/mesos/master/allocator.hpp https://reviews.apache.org/r/34614/#comment136503 Could you please add a TODO here saying that we will eventually replace 'oversubscribed' here with 'total' so that we can use this interface to update slave's regular total as well. - Jie Yu On May 22, 2015, 9:42 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34614/ --- (Updated May 22, 2015, 9:42 p.m.) Review request for mesos, Jie Yu and Niklas Nielsen. Bugs: MESOS-2730 https://issues.apache.org/jira/browse/MESOS-2730 Repository: mesos Description --- Just the stub API. Implementation is in the subsequent review. Diffs - include/mesos/master/allocator.hpp 0328ae4b2a6b39cefff3e64136f209f1593808cd src/master/allocator/mesos/allocator.hpp e6f611435969730baedf55ca475e5fa18d700b64 src/master/allocator/mesos/hierarchical.hpp 4b36d42b0c4614493562e57c5eac90c6c38ca087 src/tests/mesos.hpp 924b0ff7d307ce9650b78d1c698082c0cf4bb56c Diff: https://reviews.apache.org/r/34614/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32664: Add port mapping isolator statistics tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32664/#review85032 --- Patch looks great! Reviews applied: [34321, 34426, 34428, 34431, 34432, 32660, 34436, 34558, 32664] All tests passed. - Mesos ReviewBot On May 22, 2015, 11:45 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32664/ --- (Updated May 22, 2015, 11:45 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Add port mapping isolator statistics tests Diffs - src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/32664/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34268: stout library - adding support for Solaris
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review85054 --- Ship it! Ship It! - Stan Teresen On May 22, 2015, 7:15 p.m., Stan Teresen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 22, 2015, 7:15 p.m.) Review request for mesos, Joris Van Remoortere and Till Toenshoff. Repository: mesos-incubating Description --- stout library - adding support for Solaris Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 86949ec 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp b0ed5cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 81d64cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34268/diff/ Testing --- File Attachments adding missing new file: stout/os/sunos.hpp https://reviews.apache.org/media/uploaded/files/2015/05/15/a2e296fa-e251-4467-9873-77d8ced7f0a3__sunos.hpp Thanks, Stan Teresen
Review Request 34620: Remove unnecessary ifdefs for missing CLONE_ flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34620/ --- Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos Description --- Remove unnecessary ifdefs for missing CLONE_ flags. Diffs - src/linux/ns.hpp b695f3acf2ab8f1ba3adaa7fffd4d920107821be Diff: https://reviews.apache.org/r/34620/diff/ Testing --- Thanks, Ian Downes
Review Request 34619: Define missing CLONE_ flags for old glibc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34619/ --- Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos Description --- Define missing CLONE_ flags for old glibc. Diffs - src/linux/ns.hpp b695f3acf2ab8f1ba3adaa7fffd4d920107821be Diff: https://reviews.apache.org/r/34619/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 32664: Add port mapping isolator statistics tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32664/ --- (Updated May 22, 2015, 11:45 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Update to address review comments. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Add port mapping isolator statistics tests Diffs (updated) - src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/32664/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34616: Updated allocator to properly handle oversubscribed resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34616/ --- (Updated May 23, 2015, 12:33 a.m.) Review request for mesos, Jie Yu and Niklas Nielsen. Changes --- Added a Note and TODO. NNFR. Bugs: MESOS-2734 https://issues.apache.org/jira/browse/MESOS-2734 Repository: mesos Description --- Just ending up resuing the existing variables (Slave.total and Slave.available) to store oversubscribed resources. The nice thing is that the changes are minimal. Also, we could potentially reuse updateSlave() to also update slave's total resources in the future. Diffs (updated) - src/master/allocator/mesos/hierarchical.hpp 9c949b4d056e9ae32cc18bc812a0cdb3b73a13e5 src/tests/hierarchical_allocator_tests.cpp 1a43dc72e739f3c55787716d680faa42a7d0d86f Diff: https://reviews.apache.org/r/34616/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/#review85055 --- Patch looks great! Reviews applied: [34581] All tests passed. - Mesos ReviewBot On May 23, 2015, 3:30 a.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 23, 2015, 3:30 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/#review85045 --- Patch looks great! Reviews applied: [34581] All tests passed. - Mesos ReviewBot On May 23, 2015, 12:40 a.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 23, 2015, 12:40 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 23, 2015, 3:30 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Sorry for previous patchset - my branch was not up-to-date. Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs (updated) - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34620: Remove unnecessary ifdefs for missing CLONE_ flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34620/#review85026 --- Ship it! Ship It! - Vinod Kone On May 22, 2015, 11:39 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34620/ --- (Updated May 22, 2015, 11:39 p.m.) Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos Description --- Remove unnecessary ifdefs for missing CLONE_ flags. Diffs - src/linux/ns.hpp b695f3acf2ab8f1ba3adaa7fffd4d920107821be Diff: https://reviews.apache.org/r/34620/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34619: Define missing CLONE_ flags for old glibc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34619/#review85025 --- Ship it! Ship It! - Vinod Kone On May 22, 2015, 11:39 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34619/ --- (Updated May 22, 2015, 11:39 p.m.) Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos Description --- Define missing CLONE_ flags for old glibc. Diffs - src/linux/ns.hpp b695f3acf2ab8f1ba3adaa7fffd4d920107821be Diff: https://reviews.apache.org/r/34619/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34616: Updated allocator to properly handle oversubscribed resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34616/#review85031 --- Ship it! LGTM! Could you please add a NOTE somewhere stating that the default allocator provides DRF on the total pool of both revokable and non-revokable resources. So it's possible that a framework allocated with many non-revokable offers will unlikely to get revokable resources. We may want to experiment with using DRF separately for revokable and non-revokable resources in the future. - Jie Yu On May 22, 2015, 9:41 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34616/ --- (Updated May 22, 2015, 9:41 p.m.) Review request for mesos, Jie Yu and Niklas Nielsen. Bugs: MESOS-2734 https://issues.apache.org/jira/browse/MESOS-2734 Repository: mesos Description --- Just ending up resuing the existing variables (Slave.total and Slave.available) to store oversubscribed resources. The nice thing is that the changes are minimal. Also, we could potentially reuse updateSlave() to also update slave's total resources in the future. Diffs - src/master/allocator/mesos/hierarchical.hpp 4b36d42b0c4614493562e57c5eac90c6c38ca087 src/tests/hierarchical_allocator_tests.cpp 1a43dc72e739f3c55787716d680faa42a7d0d86f Diff: https://reviews.apache.org/r/34616/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 23, 2015, 3:27 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- s/package mesos.internal/package mesos.slave/ Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 215007b7ba8c4093ce95b79a07fd84445048b58a 3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 75ed9db54dc9ab502e978f06c55a621cacb56b91 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a538fb1a343aab039aecabe508b7747e683fd46e 3rdparty/libprocess/3rdparty/stout/include/stout/os/stat.hpp c8203d6d1ec41c1c27d139b469a21453183c4903 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 12b2e1b67df30ba4bfdbe12289ee42db8d381954 3rdparty/libprocess/include/process/process.hpp 79d1719932a3fdc90b6247d3a77adee123e72435 include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87 include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/linux/ns.hpp 6876967f4182e0cd0bc11fe124382629107eebf7 src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c73199ab2574bf7993 src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4 src/master/allocator/sorter/drf/sorter.hpp 35dc1a4d0b5e61b26a07c2c53583d75896aff27c src/master/allocator/sorter/drf/sorter.cpp ac05afdc7d408735dd796faa68c943e75540aaa7 src/master/allocator/sorter/sorter.hpp 9f7d3ccfd0994be8d562fd5efaeeb9403cf9ce94 src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025 src/tests/hierarchical_allocator_tests.cpp 85bb29e7cfc00579ff8f6d62d6c75e1b99ffcdba src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba src/tests/sorter_tests.cpp 435e0bfeb28c1a9ea124312a8b97a869945ac87f Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34545: Updated the allocator related docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34545/#review84903 --- docs/allocation-module.md https://reviews.apache.org/r/34545/#comment136342 1. s/Allocator/The allocator 2. I thought DRFD is the default and not something a 3rd party would implement as a mosule. Don't we offer it as default module implementation? docs/allocation-module.md https://reviews.apache.org/r/34545/#comment136343 s/need/needs docs/allocation-module.md https://reviews.apache.org/r/34545/#comment136344 s/the calls/calls docs/allocation-module.md https://reviews.apache.org/r/34545/#comment136345 s/reference/refer to docs/allocation-module.md https://reviews.apache.org/r/34545/#comment136346 if weights are specified in Sorter::add() Can you expand on this, please? The reader has no knowledge of what Sorter::add() might entail at this point. docs/allocation-module.md https://reviews.apache.org/r/34545/#comment136347 s/is ready/has been written docs/modules.md https://reviews.apache.org/r/34545/#comment136350 Examples: either use singular here or add a second example. - Bernd Mathiske On May 21, 2015, 7:58 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34545/ --- (Updated May 21, 2015, 7:58 a.m.) Review request for mesos, Adam B, Bernd Mathiske, and Niklas Nielsen. Bugs: MESOS-2596 https://issues.apache.org/jira/browse/MESOS-2596 Repository: mesos Description --- See summary. Diffs - docs/allocation-module.md bca54b01b80708c8265708aa9e2ef4a02fe64228 docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b docs/modules.md 835c195745421938a5dc86c3c7d5777e02d660b7 Diff: https://reviews.apache.org/r/34545/diff/ Testing --- Rendered the docs in MacDown and github. Thanks, Alexander Rukletsov
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review84908 --- First pass. 3rdparty/libprocess/include/process/http.hpp https://reviews.apache.org/r/30032/#comment136354 Alternatively you could just use a naked constant and then sizeof(buffer) below. Declaring the constant does not add any information. 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/30032/#comment136355 s/tc/tm There seems to be no call to stat here, so how can this comment be correct? 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/30032/#comment136357 Why Result and not Try? Why not propagate the error from mtime? Why snake_case and not camelCase? 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/30032/#comment136358 Why not report the error here? 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/30032/#comment136361 Can we call this in NotModified above? See also the comments there, which also apply here, since this seems to be the same code. 3rdparty/libprocess/src/tests/process_tests.cpp https://reviews.apache.org/r/30032/#comment136366 Insert blank above comment. 3rdparty/libprocess/src/tests/process_tests.cpp https://reviews.apache.org/r/30032/#comment136367 Insert blank above comment and below last line of which it applies to. irst pass - Bernd Mathiske On May 18, 2015, 9:20 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated May 18, 2015, 9:20 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp bba62b393dc863e724cb602b1504eb6517ae9730 3rdparty/libprocess/src/process.cpp e3de3cd6b536aaaf59784360aed546512dd04dc9 3rdparty/libprocess/src/tests/process_tests.cpp 67e582cc250a9767a389e2bd0cc68985477f3ffb Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 12550: Merge packaging scripts into Mesos.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12550/#review84916 --- Hi Jason, do you still want this in? - Niklas Nielsen On July 15, 2013, 10:36 a.m., Jason Dusek wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12550/ --- (Updated July 15, 2013, 10:36 a.m.) Review request for mesos. Repository: mesos-incubating Description --- The 'packaging' directory provides a home for packaging scripts, init scripts, homebrews and related code. There's some stuff for Debian and Ubuntu included. Diffs - packaging/.gitignore PRE-CREATION packaging/README.md PRE-CREATION packaging/build_mesos PRE-CREATION packaging/conf PRE-CREATION packaging/copyright PRE-CREATION packaging/debian/master.init PRE-CREATION packaging/debian/mesos.postinst PRE-CREATION packaging/debian/mesos.postrm PRE-CREATION packaging/debian/slave.init PRE-CREATION packaging/default/mesos PRE-CREATION packaging/default/mesos-master PRE-CREATION packaging/default/mesos-slave PRE-CREATION packaging/ubuntu/master.upstart PRE-CREATION packaging/ubuntu/mesos.postinst PRE-CREATION packaging/ubuntu/mesos.postrm PRE-CREATION packaging/ubuntu/slave.upstart PRE-CREATION Diff: https://reviews.apache.org/r/12550/diff/ Testing --- Thanks, Jason Dusek
Re: Review Request 34428: Extend queueing discipline wrappers to expose network isolator statistics
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34428/ --- (Updated May 22, 2015, 4:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Update to reflect review changes to Handle Bugs: MESOS-2750 https://issues.apache.org/jira/browse/MESOS-2750 Repository: mesos Description --- Extend queueing discipline wrappers to expose network isolator statistics Diffs (updated) - src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/linux/routing/handle.hpp PRE-CREATION src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00 src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34428/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 11484: Don't log every single completed task.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11484/#review84919 --- hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java is no longer hosted in our source base. Closing for now - Niklas Nielsen On June 3, 2013, 10:59 a.m., Brenden Matthews wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11484/ --- (Updated June 3, 2013, 10:59 a.m.) Review request for mesos. Repository: mesos-incubating Description --- Don't log every single completed task. On large jobs (with many thousands of tasks) this can generate an unreasonable number of log messages. Diffs - hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java afe401f5265e3d9494af7eace42eec45943184a3 Diff: https://reviews.apache.org/r/11484/diff/ Testing --- Thanks, Brenden Matthews
Re: Review Request 14039: List modules instead of using package=['.']
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14039/#review84918 --- We have ''packages': [ 'mesos' ],' in src/python/setup.py.in - do we need mesos_pb2? - Niklas Nielsen On Sept. 9, 2013, 12:22 p.m., Jason Dusek wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14039/ --- (Updated Sept. 9, 2013, 12:22 p.m.) Review request for mesos. Repository: mesos Description --- List modules instead of using package=['.'] Using packages= without an __init__.py is recommended against in the docs and happens to break FPM. From the python.org documentation: The packages option tells the Distutils to process (build, distribute, install, etc.) all pure Python modules found in each package mentioned in the packages list. In order to do this, of course, there has to be a correspondence between package names and directories in the filesystem. The default correspondence is the most obvious one, i.e. package distutils is found in the directory distutils relative to the distribution root. Thus, when you say packages = ['foo'] in your setup script, you are promising that the Distutils will find a file foo/__init__.py (which might be spelled differently on your system, but you get the idea) relative to the directory where your setup script lives. If you break this promise, the Distutils will issue a warning but still process the broken package anyway. http://docs.python.org/2/distutils/setupscript.html#listing-whole-packages Review: http://reviews.apache.org/r/14039 Diffs - src/python/setup.py.in 77fa880a16f1c8ccecb7bb0db3cfea75413b104a Diff: https://reviews.apache.org/r/14039/diff/ Testing --- Thanks, Jason Dusek
Re: Review Request 13709: Fixed typos and added .gitignore
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13709/#review84920 --- Can we split this in two? I am not sure about our policy with regards to gitignore, but the typo fixes are definitely useful! - Niklas Nielsen On Aug. 22, 2013, 12:06 a.m., Kevin Lyda wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13709/ --- (Updated Aug. 22, 2013, 12:06 a.m.) Review request for mesos. Bugs: MESOS-654 https://issues.apache.org/jira/browse/MESOS-654 Repository: mesos Description --- Fixed a few typos, added a .gitignore, removed a generated file. Diffs - .gitignore PRE-CREATION 3rdparty/libprocess/install-sh 6781b98 README e94531d src/common/values.cpp ce26119 src/linux/cgroups.cpp b97a89c src/log/coordinator.cpp 6e6466f src/log/log.cpp aea06e7 src/master/master.cpp d53b8bb src/slave/slave.cpp 92a0a7e src/tests/allocator_tests.cpp c57da6e src/tests/resources_tests.cpp 964a1b6 src/tests/status_update_manager_tests.cpp cf420e4 Diff: https://reviews.apache.org/r/13709/diff/ Testing --- git clean -fxd ./bootstrap cd build ../configure make make check All these work as they did before. git status is now clean when the tree is built (unless there have been code changes made). git diff only shows code changes, not stuff from generated files. Thanks, Kevin Lyda
Re: Review Request 11129: More cgroup killTask() logging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11129/#review84915 --- src/linux/cgroups.cpp https://reviews.apache.org/r/11129/#comment136368 This method doesn't exist anymore and the freezer has since then been refactored. Closing for now. - Niklas Nielsen On June 11, 2013, 11:43 a.m., Brenden Matthews wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11129/ --- (Updated June 11, 2013, 11:43 a.m.) Review request for mesos. Repository: mesos-incubating Description --- More cgroup killTask() logging. Review: https://reviews.apache.org/r/11129 Diffs - src/linux/cgroups.cpp 8d94fe63610c4c7a48f92d260fcd526b7a83942e Diff: https://reviews.apache.org/r/11129/diff/ Testing --- Used in production at airbnb. Thanks, Brenden Matthews
Re: Review Request 34321: Merge class Handle which is duplicated between filter/handle and queueing/handle.
On May 22, 2015, 5:28 a.m., Cong Wang wrote: src/linux/routing/handle.hpp, line 118 https://reviews.apache.org/r/34321/diff/7/?file=968966#file968966line118 Same comments near the definitions in handle.cpp, duplicated? Paul Brett wrote: Once for the declaration, second for the definition. I thought we only need one of them. - Cong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34321/#review84871 --- On May 22, 2015, 3:56 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34321/ --- (Updated May 22, 2015, 3:56 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: mesos-2665 https://issues.apache.org/jira/browse/mesos-2665 Repository: mesos Description --- Merge class Handle which is duplicated between filter/handle and queueing/handle. Diffs - src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/linux/routing/filter/basic.hpp 99b0b058633403e334e7230dfa7d705c99b3cb10 src/linux/routing/filter/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/handle.hpp 190177441bc95cf77690dbdeca189a816c6e2324 src/linux/routing/filter/icmp.hpp d558f1e31036e75780c52690e812de265ec6 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 src/linux/routing/filter/internal.hpp c74098dab97807084e6630998da354265680c763 src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76 src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa src/linux/routing/handle.hpp PRE-CREATION src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00 src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb src/linux/routing/queueing/handle.hpp 5f0cb7775f9190caba6b85cabf9019a97b2a7de2 src/linux/routing/queueing/handle.cpp cd34fc41a0b4233520a606cb439b286777c6a995 src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69 src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa Diff: https://reviews.apache.org/r/34321/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 22, 2015, 4:31 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Update to reflect changes in Handle Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description (updated) --- Report the network statistics from libnl Diffs (updated) - src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00 src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69 src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 14039: List modules instead of using package=['.']
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14039/#review84929 --- Bad patch! Reviews applied: [14039] Failed command: ./support/apply-review.sh -n -r 14039 Error: 2015-05-22 16:47:36 URL:https://reviews.apache.org/r/14039/diff/raw/ [514/514] - 14039.patch [1] error: patch failed: src/python/setup.py.in:117 error: src/python/setup.py.in: patch does not apply Failed to apply patch - Mesos ReviewBot On Sept. 9, 2013, 7:22 p.m., Jason Dusek wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14039/ --- (Updated Sept. 9, 2013, 7:22 p.m.) Review request for mesos. Repository: mesos Description --- List modules instead of using package=['.'] Using packages= without an __init__.py is recommended against in the docs and happens to break FPM. From the python.org documentation: The packages option tells the Distutils to process (build, distribute, install, etc.) all pure Python modules found in each package mentioned in the packages list. In order to do this, of course, there has to be a correspondence between package names and directories in the filesystem. The default correspondence is the most obvious one, i.e. package distutils is found in the directory distutils relative to the distribution root. Thus, when you say packages = ['foo'] in your setup script, you are promising that the Distutils will find a file foo/__init__.py (which might be spelled differently on your system, but you get the idea) relative to the directory where your setup script lives. If you break this promise, the Distutils will issue a warning but still process the broken package anyway. http://docs.python.org/2/distutils/setupscript.html#listing-whole-packages Review: http://reviews.apache.org/r/14039 Diffs - src/python/setup.py.in 77fa880a16f1c8ccecb7bb0db3cfea75413b104a Diff: https://reviews.apache.org/r/14039/diff/ Testing --- Thanks, Jason Dusek
Re: Review Request 11114: Check for Python.h in configure script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4/#review84914 --- There hasn't been any traffic on this review for almost 2 years. Closing for now - Niklas Nielsen On June 5, 2013, 7:12 p.m., Brenden Matthews wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4/ --- (Updated June 5, 2013, 7:12 p.m.) Review request for mesos. Repository: mesos-incubating Description --- Check for Python.h in configure script. Review: https://reviews.apache.org/r/4 Diffs - configure.ac 8b6d74fd78613965340ecff71bb933c9c4d24e9e Diff: https://reviews.apache.org/r/4/diff/ Testing --- Used in production at airbnb. Thanks, Brenden Matthews
Re: Review Request 34321: Merge class Handle which is duplicated between filter/handle and queueing/handle.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34321/ --- (Updated May 22, 2015, 3:56 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: mesos-2665 https://issues.apache.org/jira/browse/mesos-2665 Repository: mesos Description --- Merge class Handle which is duplicated between filter/handle and queueing/handle. Diffs (updated) - src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/linux/routing/filter/basic.hpp 99b0b058633403e334e7230dfa7d705c99b3cb10 src/linux/routing/filter/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/handle.hpp 190177441bc95cf77690dbdeca189a816c6e2324 src/linux/routing/filter/icmp.hpp d558f1e31036e75780c52690e812de265ec6 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 src/linux/routing/filter/internal.hpp c74098dab97807084e6630998da354265680c763 src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76 src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa src/linux/routing/handle.hpp PRE-CREATION src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00 src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb src/linux/routing/queueing/handle.hpp 5f0cb7775f9190caba6b85cabf9019a97b2a7de2 src/linux/routing/queueing/handle.cpp cd34fc41a0b4233520a606cb439b286777c6a995 src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69 src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa Diff: https://reviews.apache.org/r/34321/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.
On May 20, 2015, 7:34 p.m., Paul Brett wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 69 https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line69 We don't need to be root, we just need to have CAP_SYS_ADMIN, and we could pick that up through a helpful suid mount program. We call mount(2) directly and Mesos is not expected to be setuid root. This is consistent with the rest of the code so if/when we change that we'll do everything. On May 20, 2015, 7:34 p.m., Paul Brett wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 178 https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line178 This really tests if the container_path exists in the filesystem namespace, the actual location could be anywhere. True, but is it a problem if it's elsewhere? On May 20, 2015, 7:34 p.m., Paul Brett wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 203 https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line203 Should thie be comparing realpath(containerPath) with realpath(rootfs) in case the rootfs spec you are given contains symbolic links? It's a precondition that rootfs is absolute, enforced elsewhere. On May 20, 2015, 7:34 p.m., Paul Brett wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 241 https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line241 Don't we want the option of mounting read only? Nope, this is the work directory which we state is always writable. On May 20, 2015, 7:34 p.m., Paul Brett wrote: src/slave/containerizer/mesos/containerizer.cpp, line 134 https://reviews.apache.org/r/34135/diff/1/?file=957260#file957260line134 I'm sure there will be more than one linux filesystem isolator, should we call this filesystem/bind? Maybe. But I'd expect to add to the linux isolator rather than having a multitude. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/#review84644 --- On May 12, 2015, 5:47 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/ --- (Updated May 12, 2015, 5:47 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved code from Mesos Containerizer to filesystem isolators - filesystem/posix (symlinks, doesn't support container rootfs) - filesystem/linux (bind mounts, does support container rootfs) The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp b9e22e3c70bed0c29e2ca8632411789d33f779a8 src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a Diff: https://reviews.apache.org/r/34135/diff/ Testing --- existing persistent volumes tests. Thanks, Ian Downes
Re: Review Request 34137: Add support for container image provisioners.
On May 14, 2015, 12:41 p.m., Timothy Chen wrote: src/slave/containerizer/mesos/containerizer.cpp, line 193 https://reviews.apache.org/r/34137/diff/1/?file=957264#file957264line193 Not sure if I follow this logic or if I'm missing something, but the provisioners hashmap seems to be a local variable and I don't see anything populating it? And how will it contain anything? This is to support the AppC provisioner in a later review, and subsequent provisioners. On May 14, 2015, 12:41 p.m., Timothy Chen wrote: src/slave/containerizer/provisioner.hpp, line 35 https://reviews.apache.org/r/34137/diff/1/?file=957265#file957265line35 What would recover do? Don't know actually, so I removed the TODO. If needed it'll be clear. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review83824 --- On May 12, 2015, 5:47 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated May 12, 2015, 5:47 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34268: stout library - adding support for Solaris
On May 19, 2015, 12:17 a.m., Till Toenshoff wrote: Thanks a lot for this, Stan - much appreciated! There are a couple of style nits here and there and one basic question on the need of the `read`-variant for Solaris. For submitting an updated patch, please consult the patch submission guidelines http://mesos.apache.org/documentation/latest/submitting-a-patch/ specifically after Submit your patch - we need a patch that can be processed using our tooling and for that to work, an easy way is to follow that guide. Followed the guide and on git commit to the local branch I got a couple of more style recommendations which I fixed: git commit -m stout library - added support for Solaris Checking 5 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp:80: Tab found; better to use spaces [whitespace/tab] [1] 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp:110: Lines should be = 80 characters long [whitespace/line_length] [2] Total errors found: 2 But next step failed: $ support/post-reviews.py Running 'rbt post' across all of ... dbce4005a99e919fd0deaffda76e2e91fc63ade0 - (HEAD, solaris) stout library - added support for Solaris (10 minutes ago) Creating diff of: dbce4005a99e919fd0deaffda76e2e91fc63ade0 - (HEAD, solaris) stout library - added support for Solaris ... with parent diff created from: Press enter to continue or 'Ctrl-C' to skip. Failed to execute: 'rbt post --repository-url=git://git.apache.org/mesos.git --tracking-branch=master master dbce4005a99e919fd0deaffda76e2e91fc63ade0': CRITICAL: object of type 'NoneType' has no len() - Stan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review84202 --- On May 19, 2015, 12:21 a.m., Stan Teresen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 19, 2015, 12:21 a.m.) Review request for mesos, Joris Van Remoortere and Till Toenshoff. Repository: mesos-incubating Description --- stout library - adding support for Solaris Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 86949ec 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp b0ed5cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 81d64cc Diff: https://reviews.apache.org/r/34268/diff/ Testing --- File Attachments adding missing new file: stout/os/sunos.hpp https://reviews.apache.org/media/uploaded/files/2015/05/15/a2e296fa-e251-4467-9873-77d8ced7f0a3__sunos.hpp Thanks, Stan Teresen
Re: Review Request 34321: Merge class Handle which is duplicated between filter/handle and queueing/handle.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34321/#review84935 --- src/linux/routing/handle.hpp https://reviews.apache.org/r/34321/#comment136380 This is filter specific. So this should belong to linux/routing/filter/handle.hpp src/linux/routing/handle.hpp https://reviews.apache.org/r/34321/#comment136381 Please wrap the comments in 70 chars (see style guide). Here and everywhere else. src/linux/routing/handle.hpp https://reviews.apache.org/r/34321/#comment136382 This is ingress qdisc specific, so this should be moved to linux/routing/queueing/ingress.hpp I still prefer using namespace to name the handle. I.e., queueing::ingress::Handle reads well than queueing::ingress::INGRESS_HANDLE. src/linux/routing/handle.hpp https://reviews.apache.org/r/34321/#comment136384 We should avoid using non-POD global variables. So please make this a inline function (in that way, you can get rid of the cpp file). src/linux/routing/queueing/internal.hpp https://reviews.apache.org/r/34321/#comment136385 Kill this line. - Jie Yu On May 22, 2015, 3:56 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34321/ --- (Updated May 22, 2015, 3:56 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: mesos-2665 https://issues.apache.org/jira/browse/mesos-2665 Repository: mesos Description --- Merge class Handle which is duplicated between filter/handle and queueing/handle. Diffs - src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/linux/routing/filter/basic.hpp 99b0b058633403e334e7230dfa7d705c99b3cb10 src/linux/routing/filter/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/handle.hpp 190177441bc95cf77690dbdeca189a816c6e2324 src/linux/routing/filter/icmp.hpp d558f1e31036e75780c52690e812de265ec6 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 src/linux/routing/filter/internal.hpp c74098dab97807084e6630998da354265680c763 src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76 src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa src/linux/routing/handle.hpp PRE-CREATION src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00 src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb src/linux/routing/queueing/handle.hpp 5f0cb7775f9190caba6b85cabf9019a97b2a7de2 src/linux/routing/queueing/handle.cpp cd34fc41a0b4233520a606cb439b286777c6a995 src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69 src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa Diff: https://reviews.apache.org/r/34321/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34432: Remove duplicate constant string references to mesos-containerizer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34432/ --- (Updated May 22, 2015, 5:28 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Address review comment Bugs: MESOS-2754 https://issues.apache.org/jira/browse/MESOS-2754 Repository: mesos Description --- Remove duplicate constant string references to mesos-containerizer, net_tcp_rtt_... Diffs (updated) - src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/34432/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34268: stout library - adding support for Solaris
On May 19, 2015, 12:17 a.m., Till Toenshoff wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp, line 72 https://reviews.apache.org/r/34268/diff/1/?file=961220#file961220line72 Could you please explain why the standard implementation of this function would not work for Solaris? getline is a GNU C library extension and not available by default on many Unix paltforms including Solaris. STL version proposed for Solaris can be used for everything as fully portable implementation unless there are use cases where performance can be a factor. - Stan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review84202 --- On May 19, 2015, 12:21 a.m., Stan Teresen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 19, 2015, 12:21 a.m.) Review request for mesos, Joris Van Remoortere and Till Toenshoff. Repository: mesos-incubating Description --- stout library - adding support for Solaris Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 86949ec 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp b0ed5cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 81d64cc Diff: https://reviews.apache.org/r/34268/diff/ Testing --- File Attachments adding missing new file: stout/os/sunos.hpp https://reviews.apache.org/media/uploaded/files/2015/05/15/a2e296fa-e251-4467-9873-77d8ced7f0a3__sunos.hpp Thanks, Stan Teresen
Re: Review Request 12885: Slave ping timeout may be increased by flags.
On May 22, 2015, 3:56 p.m., Niklas Nielsen wrote: Is this obsoleted by https://reviews.apache.org/r/29507/? Yes, I believe so. - Brenden --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12885/#review84921 --- On July 30, 2013, 11:45 p.m., Brenden Matthews wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12885/ --- (Updated July 30, 2013, 11:45 p.m.) Review request for mesos. Repository: mesos Description --- Slave ping timeout may be increased by flags. Review: https://reviews.apache.org/r/12885 Diffs - src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954 src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23 src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8 src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80 src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c Diff: https://reviews.apache.org/r/12885/diff/ Testing --- make check Thanks, Brenden Matthews
Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/#review84944 --- Looks good! A few nits and let's get it in include/mesos/slave/oversubscription.proto https://reviews.apache.org/r/34581/#comment136407 What do you think about moving this comment into the enum? include/mesos/slave/oversubscription.proto https://reviews.apache.org/r/34581/#comment136406 Mind putting a space between comment and 'Kill'? Also, we end comments with periods :) src/Makefile.am https://reviews.apache.org/r/34581/#comment136396 The alignment is a bit off - set tabstop to 8 for Makefiles :) Here and above src/Makefile.am https://reviews.apache.org/r/34581/#comment136397 Alignment is off here too. - Niklas Nielsen On May 21, 2015, 7:31 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34581/ --- (Updated May 21, 2015, 7:31 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2760 https://issues.apache.org/jira/browse/MESOS-2760 Repository: mesos Description --- This proto describes a QoS correction message for particular executor or task. It is a generic message between QoS Controller and slave. Additionaly, updated Makefile to include this proto during compilation. This request updates the https://reviews.apache.org/r/34571/ Diffs - include/mesos/slave/oversubscription.proto PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 Diff: https://reviews.apache.org/r/34581/diff/ Testing --- * make check * run mesos: 1) build (make) 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper Thanks, Bartek Plotka
Re: Review Request 34068: The test case of extend hashmap to support custom equality and hash
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34068/#review84953 --- Patch looks great! Reviews applied: [33792, 34068] All tests passed. - Mesos ReviewBot On May 22, 2015, 5:49 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34068/ --- (Updated May 22, 2015, 5:49 p.m.) Review request for mesos, Alexander Rojas and Ben Mahler. Repository: mesos Description --- The test case of extend hashmap to support custom equality and hash Diffs - 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp e8a932e5474bf2ba1a93a945ff9bc61fb5146c02 Diff: https://reviews.apache.org/r/34068/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 34268: stout library - adding support for Solaris
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 22, 2015, 7:15 p.m.) Review request for mesos, Joris Van Remoortere and Till Toenshoff. Changes --- Uploaded a new patch which addressed the comments (post-review.py didn't fork for me) Repository: mesos-incubating Description --- stout library - adding support for Solaris Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 86949ec 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp b0ed5cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 81d64cc 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34268/diff/ Testing --- File Attachments adding missing new file: stout/os/sunos.hpp https://reviews.apache.org/media/uploaded/files/2015/05/15/a2e296fa-e251-4467-9873-77d8ced7f0a3__sunos.hpp Thanks, Stan Teresen
Re: Review Request 12885: Slave ping timeout may be increased by flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12885/#review84939 --- Bad patch! Reviews applied: [12885] Failed command: ./support/apply-review.sh -n -r 12885 Error: 2015-05-22 17:24:55 URL:https://reviews.apache.org/r/12885/diff/raw/ [6806/6806] - 12885.patch [1] error: patch failed: src/master/constants.hpp:52 error: src/master/constants.hpp: patch does not apply error: patch failed: src/master/constants.cpp:29 error: src/master/constants.cpp: patch does not apply error: patch failed: src/master/flags.hpp:25 error: src/master/flags.hpp: patch does not apply error: patch failed: src/master/master.cpp:131 error: src/master/master.cpp: patch does not apply error: patch failed: src/tests/fault_tolerance_tests.cpp:177 error: src/tests/fault_tolerance_tests.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On July 30, 2013, 11:45 p.m., Brenden Matthews wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12885/ --- (Updated July 30, 2013, 11:45 p.m.) Review request for mesos. Repository: mesos Description --- Slave ping timeout may be increased by flags. Review: https://reviews.apache.org/r/12885 Diffs - src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954 src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23 src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8 src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80 src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c Diff: https://reviews.apache.org/r/12885/diff/ Testing --- make check Thanks, Brenden Matthews
Re: Review Request 34068: The test case of extend hashmap to support custom equality and hash
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34068/ --- (Updated May 22, 2015, 5:49 p.m.) Review request for mesos, Alexander Rojas and Ben Mahler. Repository: mesos Description --- The test case of extend hashmap to support custom equality and hash Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp e8a932e5474bf2ba1a93a945ff9bc61fb5146c02 Diff: https://reviews.apache.org/r/34068/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/#review84830
---
Thanks for the efforts! Here are my main sugguestions for this patch:
1) Please split it out. Let's do step by step.
2) Introduce a top level Decipline (similar to Filter).
Please let me know if you want to chat about that.
src/linux/routing/queueing/ingress.cpp
https://reviews.apache.org/r/34426/#comment136239
Do you have this defined somewhere already?
src/linux/routing/queueing/ingress.cpp
https://reviews.apache.org/r/34426/#comment136251
First of all, I think fixing the ingress handle is OK for now. Second, if
you really want to change this, you should probably do that in a separate patch.
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136231
These causes unnessary code jumping and it's bad for readability. Could you
please revert this change? Thanks!
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136241
I prefer keeping this function simple (just getting all the qdiscs). The
filtering can be handled by `getQdisc`.
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136242
As I mentioned above, Let's keep getQdiscs simple and put all the filtering
logic in the following for loop.
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136389
Can this be in a separate patch?
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136393
Hum.. Looking at those interfaces make me feel that we should probably
introduce a top level Discipline class (not the same as the current one, but
similar to Filter):
```
template typename Config
class Discipline
{
Handle parent_;
OptionHandle handle_;
string kind_;
Config config_;
};
```
And the 'create' interface should be:
```
template typename Config
Trybool create(
const std::string _link,
const DisciplineConfig discipline)
{
...
}
```
Let's try not to return the Handle in this patch! Keep this patch smaller
makes it easier for the reviewers!
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136250
? Please do not sneak in changes like this in an already very huge diff.
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/34426/#comment136394
Let's still make the ingress::HANDLE fixed so that we don't need to change
port mapping isolator in this patch.
- Jie Yu
On May 22, 2015, 4:31 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/
---
(Updated May 22, 2015, 4:31 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2665
https://issues.apache.org/jira/browse/MESOS-2665
Repository: mesos
Description
---
Report the network statistics from libnl
Diffs
-
src/linux/routing/queueing/fq_codel.hpp
4f67ab7d64afea96a07dfcf36769a9c667749a00
src/linux/routing/queueing/fq_codel.cpp
02ad8df7814c0e549a9ca9aef39777684e6abdcb
src/linux/routing/queueing/ingress.hpp
b323a7f6daed828327d6d9e9740df81582e0ba2b
src/linux/routing/queueing/ingress.cpp
47c73376097d70819defdee31a6d1e446df6b8ba
src/linux/routing/queueing/internal.hpp
7c6c4d3d960b9a4bf44dcf482212317522353d69
src/slave/containerizer/isolators/network/port_mapping.cpp
49e983edab598e2ac487bb488fdd12840a9e7dfc
src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502
src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa
Diff: https://reviews.apache.org/r/34426/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 34431: Add htb queueing discipline
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34431/ --- (Updated May 22, 2015, 5:10 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Updated to reflect changes to Handle Bugs: MESOS-2752 https://issues.apache.org/jira/browse/MESOS-2752 Repository: mesos Description --- Add htb queueing discipline Diffs (updated) - src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/linux/routing/queueing/htb.hpp PRE-CREATION src/linux/routing/queueing/htb.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34431/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34432: Remove duplicate constant string references to mesos-containerizer
On May 22, 2015, 7:31 a.m., Ian Downes wrote: src/slave/containerizer/isolators/network/port_mapping.hpp, lines 67-73 https://reviews.apache.org/r/34432/diff/1/?file=965780#file965780line67 Please state why they are exposed. The same literarl strings are used in src/tests/port_mapping.cpp. On May 22, 2015, 7:31 a.m., Ian Downes wrote: src/slave/containerizer/isolators/network/port_mapping.cpp, lines 113-118 https://reviews.apache.org/r/34432/diff/1/?file=965781#file965781line113 Why not be consistent with the other const string inlines? Const char array is the preferred way to do it. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34432/#review84885 --- On May 21, 2015, 11:32 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34432/ --- (Updated May 21, 2015, 11:32 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2754 https://issues.apache.org/jira/browse/MESOS-2754 Repository: mesos Description --- Remove duplicate constant string references to mesos-containerizer, net_tcp_rtt_... Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/34432/diff/ Testing --- make check Thanks, Paul Brett
