Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34662/#review85336
---
Bad patch!
Reviews applied: [34662]
Failed command: make -j3 distcheck
Error:
make dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d mesos-0.23.0; then find mesos-0.23.0 -type d ! -perm -200 -exec
chmod u+w {} ';' rm -rf mesos-0.23.0 || { sleep 5 rm -rf
mesos-0.23.0; }; else :; fi
test -d mesos-0.23.0 || mkdir mesos-0.23.0
(cd 3rdparty make top_distdir=../mesos-0.23.0
distdir=../mesos-0.23.0/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd libprocess make top_distdir=../../mesos-0.23.0
distdir=../../mesos-0.23.0/3rdparty/libprocess \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[3]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
:
test -d ../../mesos-0.23.0/3rdparty/libprocess || mkdir
../../mesos-0.23.0/3rdparty/libprocess
(cd 3rdparty make top_distdir=../../../mesos-0.23.0
distdir=../../../mesos-0.23.0/3rdparty/libprocess/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd stout make top_distdir=../../../../mesos-0.23.0
distdir=../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
:
test -d ../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout || mkdir
../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
(cd include make top_distdir=../../../../../mesos-0.23.0
distdir=../../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[6]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
make[6]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
test -n : \
|| find ../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
-type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/install-sh
-c -m a+r {} {} \; \
|| chmod -R a+r
../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
make[5]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
make[4]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd include make top_distdir=../../../mesos-0.23.0
distdir=../../../mesos-0.23.0/3rdparty/libprocess/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
make[4]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
test -n : \
|| find ../../mesos-0.23.0/3rdparty/libprocess -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/install-sh
-c -m a+r {} {} \; \
|| chmod -R a+r ../../mesos-0.23.0/3rdparty/libprocess
make[3]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd src make top_distdir=../mesos-0.23.0 distdir=../mesos-0.23.0/src \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[2]: *** No rule to make target
`examples/test_resource_estimator_module.hpp', needed by `distdir'. Stop.
make[2]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[1]: *** [distdir] Error 1
make[1]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
make: ***
Re: Review Request 34676: Add printing of extended attributes to Resource objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34676/#review85335 --- Patch looks great! Reviews applied: [34676] All tests passed. - Mesos ReviewBot On May 26, 2015, 11:19 p.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34676/ --- (Updated May 26, 2015, 11:19 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2693 https://issues.apache.org/jira/browse/MESOS-2693 Repository: mesos Description --- Add printing of extended attributes to Resource objects. I have no idea what format is appropriate. I made something that stands some chance of being parseable in the future. Diffs - src/common/resources.cpp d93f38ecdc31605b6a84c8d103ac90e8f0632b01 src/tests/resources_tests.cpp 459a68bb8e30852becc7d8825a24f984cb027b08 Diff: https://reviews.apache.org/r/34676/diff/ Testing --- make check Thanks, Brian Wickman
Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
On May 27, 2015, 9 a.m., Mesos ReviewBot wrote:
Bad patch!
Reviews applied: [34662]
Failed command: make -j3 distcheck
Error:
make dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d mesos-0.23.0; then find mesos-0.23.0 -type d ! -perm -200
-exec chmod u+w {} ';' rm -rf mesos-0.23.0 || { sleep 5 rm -rf
mesos-0.23.0; }; else :; fi
test -d mesos-0.23.0 || mkdir mesos-0.23.0
(cd 3rdparty make top_distdir=../mesos-0.23.0
distdir=../mesos-0.23.0/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[2]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd libprocess make top_distdir=../../mesos-0.23.0
distdir=../../mesos-0.23.0/3rdparty/libprocess \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[3]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
:
test -d ../../mesos-0.23.0/3rdparty/libprocess || mkdir
../../mesos-0.23.0/3rdparty/libprocess
(cd 3rdparty make top_distdir=../../../mesos-0.23.0
distdir=../../../mesos-0.23.0/3rdparty/libprocess/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[4]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd stout make top_distdir=../../../../mesos-0.23.0
distdir=../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[5]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
:
test -d ../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout ||
mkdir ../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
(cd include make top_distdir=../../../../../mesos-0.23.0
distdir=../../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout/include
\
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[6]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
make[6]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
test -n : \
|| find ../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
-type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/install-sh
-c -m a+r {} {} \; \
|| chmod -R a+r
../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
make[5]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
make[4]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd include make top_distdir=../../../mesos-0.23.0
distdir=../../../mesos-0.23.0/3rdparty/libprocess/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[4]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
make[4]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
test -n : \
|| find ../../mesos-0.23.0/3rdparty/libprocess -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/install-sh
-c -m a+r {} {} \; \
|| chmod -R a+r ../../mesos-0.23.0/3rdparty/libprocess
make[3]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd src make top_distdir=../mesos-0.23.0 distdir=../mesos-0.23.0/src \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[2]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[2]: *** No rule to make target
`examples/test_resource_estimator_module.hpp', needed by `distdir'. Stop.
make[2]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[1]: *** [distdir] Error 1
make[1]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
make: *** [dist] Error 2
Re: Review Request 34529: Add non-const reference version of OptionT::get.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34529/#review85338 --- Patch looks great! Reviews applied: [34529] All tests passed. - Mesos ReviewBot On May 27, 2015, 6:40 a.m., Mark Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34529/ --- (Updated May 27, 2015, 6:40 a.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-2716 https://issues.apache.org/jira/browse/MESOS-2716 Repository: mesos Description --- Add non-const reference version of OptionT::get. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp ea79b501d9ed7b7da9636ce9c9c590738a586993 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp 7ae3b8ffc5df7f8442e72b1a10d50c3f5c373d8c Diff: https://reviews.apache.org/r/34529/diff/ Testing --- make check Thanks, Mark Wang
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review85340 --- Patch looks great! Reviews applied: [34687] All tests passed. - Mesos ReviewBot On May 26, 2015, 11:54 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated May 26, 2015, 11:54 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b src/tests/common/protobuf_utils_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On May 20, 2015, 7:10 a.m., Nikita Vetoshkin wrote:
3rdparty/libprocess/include/process/http.hpp, line 352
https://reviews.apache.org/r/30032/diff/7/?file=963338#file963338line352
strftime formatting is locale dependent. This example
```
#include locale.h
#include time.h
#include stdio.h
int main() {
char outstr[200];
time_t t;
struct tm *tmp;
const char HTTP_DATE[] = %a, %d %b %Y %T %Z;
setlocale(LC_ALL, ru_RU.UTF-8);
t = time(NULL);
tmp = localtime(t);
strftime(outstr, sizeof(outstr), HTTP_DATE, tmp);
printf(Result string is \%s\\n, outstr);
}
```
on my laptop produces
```
Result string is ??, 20 ??? 2015 10:04:43 YEKT
```
Don't know if it is an issue, maybe process should call
`setlocale(LC_ALL, C);` or add a TODO?
Nikita Vetoshkin wrote:
I think this approuch should work regardless locale being used
```
#include locale.h
#include time.h
#include stdio.h
int main() {
char outstr[200];
time_t t;
struct tm *date;
const char *WEEK_DAYS[] = {Mon, Tue, Wed, Thu, Fri, Sat,
Sun};
const char *MONTHS[] = {
Jan, Feb,
Mar, Apr, May,
Jun, Jul, Aug,
Sep, Oct, Nov,
Dec};
const char FORMAT[] = %s, %02d %s %d %02d:%02d:%02d GMT;
setlocale(LC_ALL, ru_RU.UTF-8);
t = time(NULL);
date = gmtime(t);
snprintf(outstr, sizeof(outstr), FORMAT,
WEEK_DAYS[date-tm_wday],
date-tm_mday,
MONTHS[date-tm_mon],
date-tm_year + 1900,
date-tm_hour,
date-tm_min,
date-tm_sec);
printf(Result string is \%s\\n, outstr);
}
```
Alexander Rojas wrote:
Yeah, the problem is, there are more places in the code base using
strftime, so I started a discussion in the dev list and see what happens
there.
Added your solution as part of the Time object serializers. You can check it
[here](https://reviews.apache.org/r/34703/)
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30032/#review84465
---
On May 26, 2015, 4:41 p.m., Alexander Rojas wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30032/
---
(Updated May 26, 2015, 4:41 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 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 27, 2015, 6:05 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/ --- (Updated May 27, 2015, 6:05 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 34431: Add htb queueing discipline
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34431/#review85400 --- Bad patch! Reviews applied: [34321, 34426] Failed command: ./support/apply-review.sh -n -r 34426 Error: 2015-05-27 18:07:59 URL:https://reviews.apache.org/r/34426/diff/raw/ [60225/60225] - 34426.patch [1] error: patch failed: src/linux/routing/queueing/fq_codel.cpp:64 error: src/linux/routing/queueing/fq_codel.cpp: patch does not apply error: patch failed: src/linux/routing/queueing/ingress.hpp:29 error: src/linux/routing/queueing/ingress.hpp: patch does not apply error: patch failed: src/linux/routing/queueing/ingress.cpp:63 error: src/linux/routing/queueing/ingress.cpp: patch does not apply error: patch failed: src/slave/containerizer/isolators/network/port_mapping.cpp:383 error: src/slave/containerizer/isolators/network/port_mapping.cpp: patch does not apply error: patch failed: src/tests/port_mapping_tests.cpp:1968 error: src/tests/port_mapping_tests.cpp: patch does not apply error: patch failed: src/tests/routing_tests.cpp:508 error: src/tests/routing_tests.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On May 22, 2015, 5:10 p.m., Paul Brett wrote: --- 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. Bugs: MESOS-2752 https://issues.apache.org/jira/browse/MESOS-2752 Repository: mesos Description --- Add htb queueing discipline Diffs - 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 31505: Add flow classifiers for fq_codel on egress
On May 22, 2015, 10:09 p.m., Jie Yu wrote: 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? Jie Yu wrote: Ping. Did you see these two comments? Seems that the new diff hasn't resolved these two issues. Cong Wang wrote: Yes. 1) I think we all agree on not adding a new flag in case of inconsistent behavior; 2) We already have a test case, FqCodelClassifier. Jie Yu wrote: For 1), I think we need a flag so that we can slowly roll out this feature. Otherwise, if there's a bug, all slave needs to be rebooted. For 2), I mean we need to add an integration test in port_mapping_tests.cpp to exercise the code path in the isolator. The test you mentioned is a unit test for the routing library stuff. Cong Wang wrote: 1) That depends on what is our fallback solution in case of bugs, potentially every line of new code needs a flag if not reverting to an old binary. :) So generally speaking, what is our solution when we hit some bug during deloyment? If it is reverting to the old version, then adding a flag just for deloyment is useless. Or if it is turning off some flag, then as I said every new code would potentially need a flag and some new code can't have a flag, that means this way doesn't scale. 2) In src/tests/port_mapping_tests.cpp, it calls PortMappingIsolatorProcess::create() which should cover these new code too, no? For 1), if the fallback solution is to just to restart the slave, then we usually don't require a new flag. However, in this case, we will leave filters on host eth0, and a simple restart of the slave won't clean up those filters. For 2), Yes, we exercise the code of creating those filters, but do we have ASSERT or EXPECT to check if those filters are properly setup? AFAICT, even if the filters are not properly setup, the existing tests will still finish, no? - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review84998 --- On May 26, 2015, 8:41 p.m., Cong Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/ --- (Updated May 26, 2015, 8:41 p.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/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 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 49e983edab598e2ac487bb488fdd12840a9e7dfc Diff: https://reviews.apache.org/r/31505/diff/ Testing --- Manually start two mesos containers with netperf running side. Thanks, Cong Wang
Re: Review Request 34724: Doxygen enhancements
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34724/ --- (Updated May 27, 2015, 11:38 a.m.) Review request for mesos, Ben Mahler, Joerg Schad, and Marco Massenzio. Changes --- Added example of class index Repository: mesos Description --- Removes latex generation, enables class index and docset's (for use in, for example Dash) Diffs - Doxyfile 8bba46152f59478bbd5a4573eab85ec9628316bf Diff: https://reviews.apache.org/r/34724/diff/ Testing --- File Attachments (updated) class index https://reviews.apache.org/media/uploaded/files/2015/05/27/97de9c4f-dba9-4fc0-bec2-7c2a918a341b__Screen_Shot_2015-05-27_at_11.38.12_AM.png Thanks, Niklas Nielsen
Review Request 34728: Changed callbacks to take copies rather than references in monitor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34728/ --- Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone. Repository: mesos Description --- Changed callbacks to take copies rather than references in monitor. Diffs - src/slave/monitor.cpp 12d478dd6086b32685217fdd854ffd2e05067ac4 Diff: https://reviews.apache.org/r/34728/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/#review85420 --- Patch looks great! Reviews applied: [34662] All tests passed. - Mesos ReviewBot On May 27, 2015, 4:34 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/ --- (Updated May 27, 2015, 4:34 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2650 https://issues.apache.org/jira/browse/MESOS-2650 Repository: mesos Description --- Added *ResourceEstimator* (RE) module interface. Added *TestResourceEstimator* example module. (Test *ResourceEstimator* module) Moved *TestResourceEstimator* to seperate file from *tests/mesos.hpp* Changed *oversubscription_tests* to be typed_tested (for normal RE and RE module) NOTE: The example modules were good enough for other modules' unit tests, however RE had to be extended - to push particular *Resources* to slave. In future if we implement https://issues.apache.org/jira/browse/MESOS-2764, we will be able to inject stubed *Resources* in better way. Diffs - include/mesos/module/resource_estimator.hpp PRE-CREATION src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/examples/test_resource_estimator_module.cpp PRE-CREATION src/module/manager.cpp 5fed1aa82f72aa55c8e4e946f5f458ee357162be src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba src/tests/module.hpp c379f01ae4e185960b8710b6a10626a3b8784fc8 src/tests/module.cpp c4f811998aef14ab247380318fab79f18672ab29 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 src/tests/resource_estimator.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34662/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 33876: Added usages() to resource monitor
On May 11, 2015, 3:49 p.m., Jie Yu wrote:
src/slave/monitor.cpp, line 126
https://reviews.apache.org/r/33876/diff/3/?file=955511#file955511line126
Hum, this looks like a bug to me? Since you capture `containerId` by
reference, what if by the time `onFailed` is called, the `containerId`
object is no longer valid (e.g., `monitored` has been changed because of a
new container coming in)?
Similar to that in
http://stackoverflow.com/questions/6775174/lambda-should-capturing-const-reference-by-reference-yield-undefined-behaviour
The same for 'executorInfo'.
You probably should consider do the following:
```
FutureUsage ResourceMonitorProcess::usage(ContainerID containerId)
{
...
ExecutorInfo executorInfo = monitored[containerId];
...
.onFailed([conatinerId, executorInfo](...) {})
...
}
```
Niklas Nielsen wrote:
Good catch! Should be fixed in the latest patch
Thanks!
Ian Downes wrote:
Committed code captures executorInfo by reference rather than by value as
suggested here.
This patch changes to making copies instead: https://reviews.apache.org/r/34728/
Working on a test to exercise this.
- Niklas
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33876/#review83292
---
On May 12, 2015, 1:55 p.m., Niklas Nielsen wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33876/
---
(Updated May 12, 2015, 1:55 p.m.)
Review request for mesos, Jie Yu and Vinod Kone.
Repository: mesos
Description
---
Added usages() to resource monitor to enable internal components tapping into
resource statistics.
Diffs
-
src/slave/monitor.hpp 69c60a10187f8ea617c6be9738b28e8103e0ed27
src/slave/monitor.cpp 398af010564e999b46e38560ba1e652261a9420c
Diff: https://reviews.apache.org/r/33876/diff/
Testing
---
make check
Thanks,
Niklas Nielsen
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review85380 --- 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment136923 This will result in 7 (for each day of week) + 1 (for vector itself) dynamic allocations on each call. Can/should we avoid them using C style structures? Same applies to MONTHS variable. - Nikita Vetoshkin On May 27, 2015, 2:48 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 27, 2015, 2:48 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- Adds some manipulator classes which allows formatting Time objects to ostreams. Diffs - 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
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/ --- (Updated May 27, 2015, 4:50 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Changes --- Makes the changes requiered by Bernd's review. Adds a dependency to review 34703. 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 (updated) - 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 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 27, 2015, 4:48 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- It really doesn't have a dependency. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- Adds some manipulator classes which allows formatting Time objects to ostreams. Diffs - 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review85364 --- Patch looks great! Reviews applied: [34392, 34703] All tests passed. - Mesos ReviewBot On May 27, 2015, 1:33 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 27, 2015, 1:33 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- Adds some manipulator classes which allows formatting Time objects to ostreams. Diffs - 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/ --- (Updated May 27, 2015, 4:34 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Fixed Makefile. Bugs: MESOS-2650 https://issues.apache.org/jira/browse/MESOS-2650 Repository: mesos Description --- Added *ResourceEstimator* (RE) module interface. Added *TestResourceEstimator* example module. (Test *ResourceEstimator* module) Moved *TestResourceEstimator* to seperate file from *tests/mesos.hpp* Changed *oversubscription_tests* to be typed_tested (for normal RE and RE module) NOTE: The example modules were good enough for other modules' unit tests, however RE had to be extended - to push particular *Resources* to slave. In future if we implement https://issues.apache.org/jira/browse/MESOS-2764, we will be able to inject stubed *Resources* in better way. Diffs (updated) - include/mesos/module/resource_estimator.hpp PRE-CREATION src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/examples/test_resource_estimator_module.cpp PRE-CREATION src/module/manager.cpp 5fed1aa82f72aa55c8e4e946f5f458ee357162be src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba src/tests/module.hpp c379f01ae4e185960b8710b6a10626a3b8784fc8 src/tests/module.cpp c4f811998aef14ab247380318fab79f18672ab29 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 src/tests/resource_estimator.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34662/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/common/protobuf_utils.hpp, line 83 https://reviews.apache.org/r/34687/diff/1/?file=972329#file972329line83 Shouldn't parse() go in https://github.com/apache/mesos/blob/master/src/common/parse.hpp? I don't really know - in the file there's nothing to say one way or the other: it just seems a bunch of `parse()` methods (in the `flagss::` namespace) converting from `string` to `protobuf`? I guess so, if one takes the stance the similarly named methods should all be in the same header file... I actually prefer to keep methods *logically* grouped and, ideally, the two to/from conversions next to each other, so it's easier to keep them in sync if either formats (JSON / PBuf) ought to change. Also, not particularly fond of methods kept inline and in header files - but that's a matter of taste, I can easily be convinced either way. On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/common/protobuf_utils.hpp, line 87 https://reviews.apache.org/r/34687/diff/1/?file=972329#file972329line87 We traditionally spell out variable names. Here, minfo would be masterInfo or simply 'info' fixed everywhere to `masterInfo` - awfully sorry about that. On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/tests/common/protobuf_utils_tests.cpp, line 23 https://reviews.apache.org/r/34687/diff/1/?file=972331#file972331line23 This (and standard c includes) goes first Marco Massenzio wrote: haven't we changed this recently? (I was assuming that the protobuf_utils.hpp was the 'relevant' header?) ok - fixed this (incidentally, I'd copied the order from `http_tests.cpp` which includes `gtest/gtest.h` before `vector`) I still assume that `common/protobuf_utils.hpp` can be regarded as the `related header` and should be first: correct? - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review85311 --- On May 26, 2015, 11:54 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated May 26, 2015, 11:54 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b src/tests/common/protobuf_utils_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
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/ --- (Updated May 27, 2015, 5:23 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Rebase 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 (updated) - src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 Diff: https://reviews.apache.org/r/34558/diff/ Testing --- make check Thanks, Paul Brett
Review Request 34721: Added QoS kill executor correction test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34721/ --- Review request for mesos, Bartek Plotka, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- See summary Diffs - src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 Diff: https://reviews.apache.org/r/34721/diff/ Testing --- make check (with new qos test) Thanks, Niklas Nielsen
Review Request 34720: Added kill executor correction to slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34720/ --- Review request for mesos, Bartek Plotka, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- See summary Diffs - src/slave/slave.hpp 0207eafa914e23e4c72b1e5c4fb43aae6c97049c src/slave/slave.cpp b4d20294330f791e64a597c67b686aed9de84837 Diff: https://reviews.apache.org/r/34720/diff/ Testing --- make check Thanks, Niklas Nielsen
Review Request 34719: Added QOS_KILLED as status reason
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34719/ --- Review request for mesos, Bartek Plotka, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- See summary Diffs - include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 Diff: https://reviews.apache.org/r/34719/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/common/protobuf_utils.cpp, line 169 https://reviews.apache.org/r/34687/diff/1/?file=972330#file972330line169 We have traditionally used stringify(...) if we needed type T to string conversions. Have you checked if we have one for net::IP? Marco Massenzio wrote: haven't come across it, and, trust me, I've looked for better ways of getting a string out of a net::IP object would it be in the net::IP class, or tucked somewhere else safe? sweet - found it! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review85311 --- On May 26, 2015, 11:54 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated May 26, 2015, 11:54 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b src/tests/common/protobuf_utils_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 31505: Add flow classifiers for fq_codel on egress
On May 22, 2015, 10:09 p.m., Jie Yu wrote: 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? Jie Yu wrote: Ping. Did you see these two comments? Seems that the new diff hasn't resolved these two issues. Yes. 1) I think we all agree on not adding a new flag in case of inconsistent behavior; 2) We already have a test case, FqCodelClassifier. - Cong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review84998 --- On May 26, 2015, 8:41 p.m., Cong Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/ --- (Updated May 26, 2015, 8:41 p.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/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 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 49e983edab598e2ac487bb488fdd12840a9e7dfc Diff: https://reviews.apache.org/r/31505/diff/ Testing --- Manually start two mesos containers with netperf running side. Thanks, Cong Wang
Re: Review Request 31505: Add flow classifiers for fq_codel on egress
On May 22, 2015, 10:09 p.m., Jie Yu wrote: 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? Jie Yu wrote: Ping. Did you see these two comments? Seems that the new diff hasn't resolved these two issues. Cong Wang wrote: Yes. 1) I think we all agree on not adding a new flag in case of inconsistent behavior; 2) We already have a test case, FqCodelClassifier. For 1), I think we need a flag so that we can slowly roll out this feature. Otherwise, if there's a bug, all slave needs to be rebooted. For 2), I mean we need to add an integration test in port_mapping_tests.cpp to exercise the code path in the isolator. The test you mentioned is a unit test for the routing library stuff. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review84998 --- On May 26, 2015, 8:41 p.m., Cong Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/ --- (Updated May 26, 2015, 8:41 p.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/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 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 49e983edab598e2ac487bb488fdd12840a9e7dfc Diff: https://reviews.apache.org/r/31505/diff/ Testing --- Manually start two mesos containers with netperf running side. Thanks, Cong Wang
Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
Thx, found the issue (:
2015-05-27 8:39 GMT-07:00 Niklas Nielsen nik...@mesosphere.io:
+1 to Vinod's suggestion. The buildbot does a dist build, so also make
sure new public headers are in src/Makefile.am
On 27 May 2015 at 08:32, Vinod Kone vi...@twitter.com.invalid wrote:
You likely forgot to add the file to your patch.
@vinodkone
On May 27, 2015, at 2:08 AM, Bartek Plotka bwplo...@gmail.com wrote:
On May 27, 2015, 9 a.m., Mesos ReviewBot wrote:
Bad patch!
Reviews applied: [34662]
Failed command: make -j3 distcheck
Error:
make dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d mesos-0.23.0; then find mesos-0.23.0 -type d ! -perm
-200 -exec chmod u+w {} ';' rm -rf mesos-0.23.0 || { sleep 5 rm -rf
mesos-0.23.0; }; else :; fi
test -d mesos-0.23.0 || mkdir mesos-0.23.0
(cd 3rdparty make top_distdir=../mesos-0.23.0
distdir=../mesos-0.23.0/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[2]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd libprocess make top_distdir=../../mesos-0.23.0
distdir=../../mesos-0.23.0/3rdparty/libprocess \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[3]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
:
test -d ../../mesos-0.23.0/3rdparty/libprocess || mkdir
../../mesos-0.23.0/3rdparty/libprocess
(cd 3rdparty make top_distdir=../../../mesos-0.23.0
distdir=../../../mesos-0.23.0/3rdparty/libprocess/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[4]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd stout make top_distdir=../../../../mesos-0.23.0
distdir=../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[5]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
:
test -d ../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
|| mkdir ../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
(cd include make top_distdir=../../../../../mesos-0.23.0
distdir=../../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout/include
\
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[6]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
make[6]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
test -n : \
|| find
../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout -type d !
-perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/install-sh
-c -m a+r {} {} \; \
|| chmod -R a+r
../../../../mesos-0.23.0/3rdparty/libprocess/3rdparty/stout
make[5]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
make[4]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd include make top_distdir=../../../mesos-0.23.0
distdir=../../../mesos-0.23.0/3rdparty/libprocess/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[4]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
make[4]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
test -n : \
|| find ../../mesos-0.23.0/3rdparty/libprocess -type d ! -perm
-755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/install-sh
-c -m a+r {} {} \; \
|| chmod -R a+r ../../mesos-0.23.0/3rdparty/libprocess
make[3]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: Leaving directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd src make top_distdir=../mesos-0.23.0
distdir=../mesos-0.23.0/src \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=:
distdir)
make[2]: Entering directory
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[2]: *** No rule to make target
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/#review85384 --- Patch looks great! Reviews applied: [34392, 34703, 30032] All tests passed. - Mesos ReviewBot On May 27, 2015, 2:50 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated May 27, 2015, 2:50 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 31505: Add flow classifiers for fq_codel on egress
On May 22, 2015, 10:09 p.m., Jie Yu wrote: 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? Ping. Did you see these two comments? Seems that the new diff hasn't resolved these two issues. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review84998 --- On May 26, 2015, 8:41 p.m., Cong Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/ --- (Updated May 26, 2015, 8:41 p.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/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 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 49e983edab598e2ac487bb488fdd12840a9e7dfc Diff: https://reviews.apache.org/r/31505/diff/ Testing --- Manually start two mesos containers with netperf running side. Thanks, Cong Wang
Re: Review Request 34654: Send docker inspect output with TaskStatus data.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34654/#review85390 --- Ship it! This looks good but I'd like to can clean up the 'output' part and just use the JSON that gets passed in. src/docker/docker.hpp https://reviews.apache.org/r/34654/#comment136944 My expectation based on our design discussions was that we were going to save just the JSON and then send the stringified version of the JSON back in the 'TaskStatus.data'. While saving the output from 'docker inspect' effectively does the same thing it seems weird to pass two parameters to the create function that represent the same data. Why not just hold on to the JSON as a field 'json' here and then stringify that in docker/executor.cpp? The thing that was especially wierd to me about the current code is that it relies on the invariant in our 'docker inspect' that the output only has an array of 1 JSON object which isn't captured in the code or as an invariant and if ever changed could cause a subtle bug. Maybe I'm missing something though? src/docker/executor.cpp https://reviews.apache.org/r/34654/#comment136940 I'm assuming that you're just delaying sending the TASK_RUNNING until 'inspect' properly returns? Can you comment as much? I don't think it's obvious that calling inspect satisfies this requirement. src/docker/executor.cpp https://reviews.apache.org/r/34654/#comment136941 Why back to MergeFrom instead of CopyFrom? src/docker/executor.cpp https://reviews.apache.org/r/34654/#comment136938 You should be able to drop the parameter all together, in the lambdas below as well. Can you give that a shot and let me know if it doesn't work please? src/docker/executor.cpp https://reviews.apache.org/r/34654/#comment136937 Can we capture this as a constant please? src/docker/executor.cpp https://reviews.apache.org/r/34654/#comment136934 -2 on each line here. - Benjamin Hindman On May 26, 2015, 6:14 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34654/ --- (Updated May 26, 2015, 6:14 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-2368 https://issues.apache.org/jira/browse/MESOS-2368 Repository: mesos Description --- Send docker inspect output with TaskStatus data. Diffs - src/docker/docker.hpp d06c73a src/docker/docker.cpp ee74da5 src/docker/executor.cpp 075c6b5 src/tests/docker_containerizer_tests.cpp 7524803 Diff: https://reviews.apache.org/r/34654/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 34141: AppC provsioning backend.
On May 18, 2015, 4:38 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc/backend.hpp, line 50 https://reviews.apache.org/r/34141/diff/1/?file=957281#file957281line50 Not a big deal, but would it be more flexible for Bankend to take a rootImage as an argument instead of a vector, which is already flattened? Resolving an dependency tree could yield a list of all tree nodes for the copy backend. I want Backends to be simple and deal only with the flattened list of layers. This could be changed in the future if smarter Backends are implemented. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34141/#review84237 --- 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/34141/ --- (Updated May 12, 2015, 5:48 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Simple copy backend that forms the rootfs by copying each layer. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34141/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 27, 2015, 9:04 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 31505: Add flow classifiers for fq_codel on egress
On May 22, 2015, 10:09 p.m., Jie Yu wrote: 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? Jie Yu wrote: Ping. Did you see these two comments? Seems that the new diff hasn't resolved these two issues. Cong Wang wrote: Yes. 1) I think we all agree on not adding a new flag in case of inconsistent behavior; 2) We already have a test case, FqCodelClassifier. Jie Yu wrote: For 1), I think we need a flag so that we can slowly roll out this feature. Otherwise, if there's a bug, all slave needs to be rebooted. For 2), I mean we need to add an integration test in port_mapping_tests.cpp to exercise the code path in the isolator. The test you mentioned is a unit test for the routing library stuff. Cong Wang wrote: 1) That depends on what is our fallback solution in case of bugs, potentially every line of new code needs a flag if not reverting to an old binary. :) So generally speaking, what is our solution when we hit some bug during deloyment? If it is reverting to the old version, then adding a flag just for deloyment is useless. Or if it is turning off some flag, then as I said every new code would potentially need a flag and some new code can't have a flag, that means this way doesn't scale. 2) In src/tests/port_mapping_tests.cpp, it calls PortMappingIsolatorProcess::create() which should cover these new code too, no? Jie Yu wrote: For 1), if the fallback solution is to just to restart the slave, then we usually don't require a new flag. However, in this case, we will leave filters on host eth0, and a simple restart of the slave won't clean up those filters. For 2), Yes, we exercise the code of creating those filters, but do we have ASSERT or EXPECT to check if those filters are properly setup? AFAICT, even if the filters are not properly setup, the existing tests will still finish, no? 1) We only leave the default filters, which is used to catch all non-matched packets (essentially a kenrel issue as mentioned in comments), so nothing harms here. 2) Test cases like ContainerICMPExternal check the return value of ::create() and ::isolate(), so if any of these filters fails to setup, then they should fail, right? I mean: // Isolate the forked child. AWAIT_READY(isolator.get()-isolate(containerId, pid.get())); Even if not, we should fix these test cases rather adding a new one? Since they are supposed to catch all tc setup failures? - Cong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review84998 --- On May 26, 2015, 8:41 p.m., Cong Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/ --- (Updated May 26, 2015, 8:41 p.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/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c src/linux/routing/filter/filter.hpp 024582cf8274da2e5b4ebd00fcf83d6930e2 src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76 src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e
Re: Review Request 34730: Added 'updateSlave()' in master to handle oversubscribed resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34730/#review85468 --- Ship it! Taken it is a stub patch src/master/master.cpp https://reviews.apache.org/r/34730/#comment137046 Was there some additional math, only to rescind if the estimate is lower? If not now, should we add a todo? - Niklas Nielsen On May 27, 2015, 4:11 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34730/ --- (Updated May 27, 2015, 4:11 p.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Bugs: MESOS-2733 https://issues.apache.org/jira/browse/MESOS-2733 Repository: mesos Description --- This is mostly a stub with a metric for the messages. The actual plumbing to the allocator will be sent in a subsequent review. Diffs - src/master/master.hpp c8c6251e8db59c814bc92766df7b05e29d38e97e src/master/master.cpp 1526f59e7c6b135657550eab2ca46216923a01f6 src/master/metrics.hpp 78d066619a688411550fe9a78811337f3b06abaf src/master/metrics.cpp ee09664c3c28abb3fdc5e8f2713799ee6a25661b src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 Diff: https://reviews.apache.org/r/34730/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/ --- (Updated May 28, 2015, 12:04 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Refactored RETypes Bugs: MESOS-2650 https://issues.apache.org/jira/browse/MESOS-2650 Repository: mesos Description --- Added *ResourceEstimator* (RE) module interface. Added *TestResourceEstimator* example module. (Test *ResourceEstimator* module) Moved *TestResourceEstimator* to seperate file from *tests/mesos.hpp* Changed *oversubscription_tests* to be typed_tested (for normal RE and RE module) NOTE: The example modules were good enough for other modules' unit tests, however RE had to be extended - to push particular *Resources* to slave. In future if we implement https://issues.apache.org/jira/browse/MESOS-2764, we will be able to inject stubed *Resources* in better way. Diffs (updated) - include/mesos/module/resource_estimator.hpp PRE-CREATION src/Makefile.am 28669f6d2d1a6a9f616ff4a288b9038235a23595 src/examples/test_resource_estimator_module.cpp PRE-CREATION src/module/manager.cpp 5fed1aa82f72aa55c8e4e946f5f458ee357162be src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba src/tests/module.hpp c379f01ae4e185960b8710b6a10626a3b8784fc8 src/tests/module.cpp c4f811998aef14ab247380318fab79f18672ab29 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 src/tests/resource_estimator.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34662/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 34729: Updated slave to send total amount of oversubscribed resources.
On May 27, 2015, 11:44 p.m., Jie Yu wrote: src/messages/messages.proto, lines 339-342 https://reviews.apache.org/r/34729/diff/1/?file=973027#file973027line339 It might not be an issue right now, but once we start to support updating slave's total regular resources, how can we distinguish between the following two cases: 1) We don't want to update oversubscribed resources 2) The new updated oversubscribed resources are zero I am thinking for 1) the slave should send the previous value if it doesn't want to (or have an) update. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34729/#review85463 --- On May 27, 2015, 11:11 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34729/ --- (Updated May 27, 2015, 11:11 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, and Brian Wickman. Bugs: MESOS-2770 https://issues.apache.org/jira/browse/MESOS-2770 Repository: mesos Description --- Per the latest design, the slave needs to send the total amount of oversubscribed resources (both allocated and available). Also renamed the message to UpdateSlave to make it more generic (e.g., can be used in the future to update slave's total resources too). Diffs - src/messages/messages.proto 39dac721fbe5b97842dd5d1d68cc135148ae02a2 src/slave/flags.hpp 6ca59dc9fc748ec738259406642ec17c0752590c src/slave/flags.cpp a8c7c498d674ca832fa052412a373c9ace4b3fc3 src/slave/slave.hpp 0207eafa914e23e4c72b1e5c4fb43aae6c97049c src/slave/slave.cpp b4d20294330f791e64a597c67b686aed9de84837 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 Diff: https://reviews.apache.org/r/34729/diff/ Testing --- make check Will add a test in a subsequent review when a framework is able to launch a revocable task. Thanks, Vinod Kone
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review85499 --- Patch looks great! Reviews applied: [34687] All tests passed. - Mesos ReviewBot On May 27, 2015, 6:07 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated May 27, 2015, 6:07 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b src/tests/common/protobuf_utils_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 34729: Updated slave to send total amount of oversubscribed resources.
On May 27, 2015, 11:44 p.m., Jie Yu wrote: src/messages/messages.proto, lines 339-342 https://reviews.apache.org/r/34729/diff/1/?file=973027#file973027line339 It might not be an issue right now, but once we start to support updating slave's total regular resources, how can we distinguish between the following two cases: 1) We don't want to update oversubscribed resources 2) The new updated oversubscribed resources are zero Vinod Kone wrote: I am thinking for 1) the slave should send the previous value if it doesn't want to (or have an) update. Good idea! SGTM. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34729/#review85463 --- On May 27, 2015, 11:11 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34729/ --- (Updated May 27, 2015, 11:11 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, and Brian Wickman. Bugs: MESOS-2770 https://issues.apache.org/jira/browse/MESOS-2770 Repository: mesos Description --- Per the latest design, the slave needs to send the total amount of oversubscribed resources (both allocated and available). Also renamed the message to UpdateSlave to make it more generic (e.g., can be used in the future to update slave's total resources too). Diffs - src/messages/messages.proto 39dac721fbe5b97842dd5d1d68cc135148ae02a2 src/slave/flags.hpp 6ca59dc9fc748ec738259406642ec17c0752590c src/slave/flags.cpp a8c7c498d674ca832fa052412a373c9ace4b3fc3 src/slave/slave.hpp 0207eafa914e23e4c72b1e5c4fb43aae6c97049c src/slave/slave.cpp b4d20294330f791e64a597c67b686aed9de84837 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 Diff: https://reviews.apache.org/r/34729/diff/ Testing --- make check Will add a test in a subsequent review when a framework is able to launch a revocable task. Thanks, Vinod Kone
Review Request 34748: Defined protobuf for usage returned by Resource Monitor. Reused ResourceUsage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34748/ --- Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2772 https://issues.apache.org/jira/browse/MESOS-2772 Repository: mesos Description --- Changed ResourceMonitor to use ResourceUsage instead of ResourceMonitor::Usage. Reused old ResoureUsage in message mesos.proto NOTE: That is required for modules which need to fetch ResourceUsage e.g ResourceEstimator and QoSController. Discussed that ResourceUsage message proto is free to reuse and modify, since it's not use anywhere else. Diffs - include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 src/slave/monitor.hpp 48fe59faacf53f1015665b0867aefd08d5b29225 src/slave/monitor.cpp a5a52b116ec38535dcc23e720e8a3a1fbc73762e Diff: https://reviews.apache.org/r/34748/diff/ Testing --- make check Thanks, Bartek Plotka
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/#review85512 --- Bad patch! Reviews applied: [32664] Failed command: ./support/apply-review.sh -n -r 32664 Error: 2015-05-28 05:19:18 URL:https://reviews.apache.org/r/32664/diff/raw/ [42205/42205] - 32664.patch [1] error: patch failed: src/tests/port_mapping_tests.cpp:2015 error: src/tests/port_mapping_tests.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On May 27, 2015, 9:04 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32664/ --- (Updated May 27, 2015, 9:04 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 34729: Updated slave to send total amount of oversubscribed resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34729/#review85462 --- Ship it! Ship It! - Jie Yu On May 27, 2015, 11:11 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34729/ --- (Updated May 27, 2015, 11:11 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, and Brian Wickman. Bugs: MESOS-2770 https://issues.apache.org/jira/browse/MESOS-2770 Repository: mesos Description --- Per the latest design, the slave needs to send the total amount of oversubscribed resources (both allocated and available). Also renamed the message to UpdateSlave to make it more generic (e.g., can be used in the future to update slave's total resources too). Diffs - src/messages/messages.proto 39dac721fbe5b97842dd5d1d68cc135148ae02a2 src/slave/flags.hpp 6ca59dc9fc748ec738259406642ec17c0752590c src/slave/flags.cpp a8c7c498d674ca832fa052412a373c9ace4b3fc3 src/slave/slave.hpp 0207eafa914e23e4c72b1e5c4fb43aae6c97049c src/slave/slave.cpp b4d20294330f791e64a597c67b686aed9de84837 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 Diff: https://reviews.apache.org/r/34729/diff/ Testing --- make check Will add a test in a subsequent review when a framework is able to launch a revocable task. Thanks, Vinod Kone
Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
On May 28, 2015, 12:24 a.m., Vinod Kone wrote: src/examples/test_resource_estimator_module.cpp, lines 40-44 https://reviews.apache.org/r/34662/diff/6/?file=973211#file973211line40 I'm confused. 'parameters' are unused and the passed 'flags' are empty? Niklas Nielsen wrote: TestResourceEstimator need to have a 'create()' which takes a Flags object, to be consistent with modules typed tests; so it's is just to avoid another 'create()'. Would you prefer that? Parameters could be removed - @Bartek Plotka, mind removing that one? :) Bartek Plotka wrote: sure (: Bartek Plotka wrote: However we would need to create new interface method for Module if we don't want use parameters in that case... @vinod and @niklas please see test_allocator_module.cpp it's quite common because of the convention. Even that parameters are not used. - Bartek --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/#review85473 --- On May 28, 2015, 1:20 a.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/ --- (Updated May 28, 2015, 1:20 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2650 https://issues.apache.org/jira/browse/MESOS-2650 Repository: mesos Description --- Added *ResourceEstimator* (RE) module interface. Added *TestResourceEstimator* example module. (Test *ResourceEstimator* module) Moved *TestResourceEstimator* to seperate file from *tests/mesos.hpp* Changed *oversubscription_tests* to be typed_tested (for normal RE and RE module) NOTE: The example modules were good enough for other modules' unit tests, however RE had to be extended - to push particular *Resources* to slave. In future if we implement https://issues.apache.org/jira/browse/MESOS-2764, we will be able to inject stubed *Resources* in better way. Diffs - include/mesos/module/resource_estimator.hpp PRE-CREATION src/Makefile.am 28669f6d2d1a6a9f616ff4a288b9038235a23595 src/examples/test_resource_estimator_module.cpp PRE-CREATION src/module/manager.cpp 5fed1aa82f72aa55c8e4e946f5f458ee357162be src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba src/tests/module.hpp c379f01ae4e185960b8710b6a10626a3b8784fc8 src/tests/module.cpp c4f811998aef14ab247380318fab79f18672ab29 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 src/tests/resource_estimator.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34662/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 34748: Defined protobuf for usage returned by Resource Monitor. Reused ResourceUsage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34748/ --- (Updated May 28, 2015, 2:37 a.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2772 https://issues.apache.org/jira/browse/MESOS-2772 Repository: mesos Description (updated) --- Changed ResourceMonitor to use ResourceUsage instead of ResourceMonitor::Usage. Reused old ResoureUsage in message mesos.proto NOTE: That is required for modules which need to fetch ResourceUsage e.g ResourceEstimator and QoSController. Discussed that message ResourceUsage in mesos.proto is free to reuse and modify, since it's not used anywhere else. Diffs - include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 src/slave/monitor.hpp 48fe59faacf53f1015665b0867aefd08d5b29225 src/slave/monitor.cpp a5a52b116ec38535dcc23e720e8a3a1fbc73762e Diff: https://reviews.apache.org/r/34748/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 34703: Added stream manipulators for the Time object.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34703/#review85514
---
3rdparty/libprocess/include/process/time.hpp
https://reviews.apache.org/r/34703/#comment137084
Can I add one more nitpick? If we make `WEEK_DAYS` and `MONTHS` `static`
then will put only array (which is initialised staticly and save in binary)
pointer on stack instead of recreating array every time.
Here's my code:
```
const char *WEEK_DAYS[] = {Sun, Mon, Tue, Wed, Thu, Fri, Sat};
std::cout WEEK_DAYS[i] std::endl;
```
Here's a relevant snippet of g++ assempler output:
```
movq$.LC0, (%rsp)
movq$.LC1, 8(%rsp)
movq$.LC2, 16(%rsp)
movq$.LC3, 24(%rsp)
movq$.LC4, 32(%rsp)
movq$.LC5, 40(%rsp)
movq$.LC6, 48(%rsp)
movq(%rsp,%rdi,8), %rsi
movl$_ZSt4cout, %edi
```
If we make it `const`, then it starts looking like this:
```
movq_ZZ4mainE9WEEK_DAYS(,%rdi,8), %rsi
movl$_ZSt4cout, %edi
```
- Nikita Vetoshkin
On May 27, 2015, 3:46 p.m., Alexander Rojas wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34703/
---
(Updated May 27, 2015, 3:46 p.m.)
Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and
Till Toenshoff.
Bugs: mesos-708
https://issues.apache.org/jira/browse/mesos-708
Repository: mesos
Description
---
Adds some manipulator classes which allows formatting Time objects to
ostreams.
Diffs
-
3rdparty/libprocess/include/process/time.hpp
c5ab2a3cfa83590eb6612152ae365dd67f51cea9
3rdparty/libprocess/src/tests/time_tests.cpp
be314182c65c05d439b81aa5248a71d93f6f0a0b
Diff: https://reviews.apache.org/r/34703/diff/
Testing
---
make check
Thanks,
Alexander Rojas
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review85456 --- Disk usage by the store is currently unbounded. Do we need to add the ability to discover or limit the size of the image store? Will you limit the number of fetchers that can be run concurrently to limit the performance impact to existing containers? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137028 And log the error? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137029 Does the mesos-fetcher choose the name or is that down to the requester? Can we assume that the name is safe to use? Why not pass it from StoreProcess rather than try to work it out after the fact? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137030 We probably want to keep a track on how long it took so we can debug performance issues. src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137031 Do we cache the hashes? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137032 Why not do the decompress, hash untar as a pipeline to reduce disk usage? - Paul Brett On May 26, 2015, 6:25 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 26, 2015, 6:25 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
Re: Review Request 34737: Added test to verify fix for MESOS-2771
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34737/ --- (Updated May 27, 2015, 4:14 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone. Bugs: MESOS-2771 https://issues.apache.org/jira/browse/MESOS-2771 Repository: mesos Description --- Added test to verify fix for MESOS-2771 Diffs - src/tests/monitor_tests.cpp ca3b7f4a85824697a4e6701285f69ba337506147 Diff: https://reviews.apache.org/r/34737/diff/ Testing --- Test verifies fix for MESOS-2771, where references to executor infos and container ids could get invalidated if monitoring of a container stopped during a statistics collection. Tested with make check before and after fix. Before: I0527 15:51:57.943672 388640768 process.cpp:2201] Cleaning up __http__(1)@10.0.68.255:50624 I0527 15:51:57.943717 387567616 process.cpp:2094] Resuming __gc__@10.0.68.255:50624 at 2015-05-27 22:51:57.943728896+00:00 PC: @0x11122bb3d std::__1::operator () *** SIGSEGV (@0x0) received by PID 42293 (TID 0x7fff7cdb2300) stack trace: *** @ 0x7fff8eaebf1a _sigtramp @ 0x7fd785800ee8 (unknown) @0x1112e97d0 mesos::operator() @0x111b41731 mesos::internal::slave::ResourceMonitorProcess::usage()::$_0::operator()() @0x111b415ed _ZZNK7process6FutureIN5mesos8internal5slave15ResourceMonitor5UsageEE8onFailedIZNS3_22ResourceMonitorProcess5usageENS1_11ContainerIDEE3$_0vEERKS6_OT_NS6_6PreferEENUlRKNSt3__112basic_stringIcNSG_11char_traitsIcEENSG_9allocatorIcE_clESO_ @0x111b4152c _ZNSt3__110__function6__funcIZNK7process6FutureIN5mesos8internal5slave15ResourceMonitor5UsageEE8onFailedIZNS6_22ResourceMonitorProcess5usageENS4_11ContainerIDEE3$_0vEERKS9_OT_NS9_6PreferEEUlRKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcE_NSM_ISR_EEFvSQ_EEclESQ_ @0x112106544 std::__1::function::operator()() @0x1112067ab _ZN7process8internal3runINSt3__18functionIFvRKNS2_12basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEJRS9_EEEvRKNS2_6vectorIT_NS7_ISG_DpOT0_ @0x111b86400 process::Future::fail() @0x111ba05bd process::Promise::fail() @0x111b9722c process::internal::thenf() @0x111b9f823 _ZNSt3__110__function6__funcINS_6__bindIPFvRKNS_8functionIFN7process6FutureIN5mesos8internal5slave15ResourceMonitor5UsageEEERKNS6_18ResourceStatisticsRKNS_10shared_ptrINS4_7PromiseISA_RKNS5_ISC_EEEJSI_RSM_RNS_12placeholders4__phILi1EENS_9allocatorISZ_EEFvSR_EEclESR_ @0x1121db254 std::__1::function::operator()() @0x111b9c90d _ZZNK7process6FutureIN5mesos18ResourceStatisticsEE5onAnyIRNSt3__18functionIFvRKS3_EEEvEES8_OT_NS3_6PreferEENUlS8_E_clES8_ @0x111b9c84c _ZNSt3__110__function6__funcIZNK7process6FutureIN5mesos18ResourceStatisticsEE5onAnyIRNS_8functionIFvRKS6_EEEvEESA_OT_NS6_6PreferEEUlSA_E_NS_9allocatorISH_EESB_EclESA_ @0x10e73b0d4 std::__1::function::operator()() @0x10e73a50b _ZN7process8internal3runINSt3__18functionIFvRKNS_6FutureIN5mesos18ResourceStatisticsEEJRS7_EEEvRKNS2_6vectorIT_NS2_9allocatorISE_DpOT0_ @0x10efd83c1 process::Future::fail() @0x10efdd49d _ZZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINSt3__16__bindIMS3_FbRKNS5_12basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEJRS3_RNS5_12placeholders4__phILi1EEbEERKS3_OT_NS3_6PreferEENUlSE_E_clESE_ @0x10efdd1fc _ZNSt3__110__function6__funcIZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINS_6__bindIMS6_FbRKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEJRS6_RNS_12placeholders4__phILi1EEbEERKS6_OT_NS6_6PreferEEUlSG_E_NSC_ISU_EEFvSG_EEclESG_ @0x10e5b2304 std::__1::function::operator()() @0x10efdb9f9 process::Future::onFailed() @0x10efdb6c8 _ZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINSt3__16__bindIMS3_FbRKNS5_12basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEJRS3_RNS5_12placeholders4__phILi1EEbEERKS3_OT_NS3_6PreferE @0x10efd9145 _ZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINSt3__16__bindIMS3_FbRKNS5_12basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEJRS3_RNS5_12placeholders4__phILi1RKS3_OT_ @0x10efd8b43 process::Promise::associate() @0x10efd81bd process::Promise::set() @0x10efd684e mesos::internal::tests::MonitorTest_UsageFailure_Test::TestBody() @0x10f4155b3 testing::internal::HandleSehExceptionsInMethodIfSupported() @0x10f3fd667 testing::internal::HandleExceptionsInMethodIfSupported() @0x10f3d7135 testing::Test::Run() @0x10f3d80eb testing::TestInfo::Run() @0x10f3d8cd7 testing::TestCase::Run() ^C^C [1]42293 segmentation
Re: Review Request 34724: Doxygen enhancements
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34724/#review85460 --- Ship it! Awesome! Doxyfile https://reviews.apache.org/r/34724/#comment137038 Apache Mesos Docs, maybe? - Marco Massenzio On May 27, 2015, 6:38 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34724/ --- (Updated May 27, 2015, 6:38 p.m.) Review request for mesos, Ben Mahler, Joerg Schad, and Marco Massenzio. Repository: mesos Description --- Removes latex generation, enables class index and docset's (for use in, for example Dash) Diffs - Doxyfile 8bba46152f59478bbd5a4573eab85ec9628316bf Diff: https://reviews.apache.org/r/34724/diff/ Testing --- File Attachments class index https://reviews.apache.org/media/uploaded/files/2015/05/27/97de9c4f-dba9-4fc0-bec2-7c2a918a341b__Screen_Shot_2015-05-27_at_11.38.12_AM.png Thanks, Niklas Nielsen
Re: Review Request 34662: Modularized ResourceEstimator and added test for RE module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/#review85413 --- Do you want to wire up module loading in src/slave/resource_estimator.cpp in this patch, or in a follow up? :) src/tests/oversubscription_tests.cpp https://reviews.apache.org/r/34662/#comment137018 When you resolved the conflict by wrapping the test resource estimator in the new namespace, you shouldn't need 'mesos::internal::tests::ModuleID::' src/tests/oversubscription_tests.cpp https://reviews.apache.org/r/34662/#comment137019 Let's align like in master_allocator_tests.cpp src/tests/oversubscription_tests.cpp https://reviews.apache.org/r/34662/#comment136975 Reinsert newline please :) src/tests/resource_estimator.hpp https://reviews.apache.org/r/34662/#comment137020 Max line length for comments is 70 cols :) src/tests/resource_estimator.hpp https://reviews.apache.org/r/34662/#comment137026 Does it make sense to use the 'slave' namespace instead of introducing a new one? src/tests/resource_estimator.hpp https://reviews.apache.org/r/34662/#comment137021 End with period :) src/tests/resource_estimator.hpp https://reviews.apache.org/r/34662/#comment137022 Sorry Bartek, I know I asked for the string version of the factory. I thought we could get rid of the flags based one. Let's kill this and use the slave flags in test_resource_estimator_module.cpp Can you kill this, if you have the 'factory' below? - Niklas Nielsen On May 27, 2015, 9:34 a.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34662/ --- (Updated May 27, 2015, 9:34 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2650 https://issues.apache.org/jira/browse/MESOS-2650 Repository: mesos Description --- Added *ResourceEstimator* (RE) module interface. Added *TestResourceEstimator* example module. (Test *ResourceEstimator* module) Moved *TestResourceEstimator* to seperate file from *tests/mesos.hpp* Changed *oversubscription_tests* to be typed_tested (for normal RE and RE module) NOTE: The example modules were good enough for other modules' unit tests, however RE had to be extended - to push particular *Resources* to slave. In future if we implement https://issues.apache.org/jira/browse/MESOS-2764, we will be able to inject stubed *Resources* in better way. Diffs - include/mesos/module/resource_estimator.hpp PRE-CREATION src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/examples/test_resource_estimator_module.cpp PRE-CREATION src/module/manager.cpp 5fed1aa82f72aa55c8e4e946f5f458ee357162be src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba src/tests/module.hpp c379f01ae4e185960b8710b6a10626a3b8784fc8 src/tests/module.cpp c4f811998aef14ab247380318fab79f18672ab29 src/tests/oversubscription_tests.cpp 75c25b04c1e6a8e0e7e8fd55440743fe1699af88 src/tests/resource_estimator.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34662/diff/ Testing --- make check Thanks, Bartek Plotka
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/Makefile.am, line 1387 https://reviews.apache.org/r/34687/diff/1/?file=972328#file972328line1387 Is the alignment right here? Try to set your tabstop to 8 :) Also, shouldn't the ordering bring this further down? no, it's utterly wrong - I planned to fix it later using vim (Eclipse is all off with tabs) but forgot. On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/common/protobuf_utils.cpp, line 169 https://reviews.apache.org/r/34687/diff/1/?file=972330#file972330line169 We have traditionally used stringify(...) if we needed type T to string conversions. Have you checked if we have one for net::IP? haven't come across it, and, trust me, I've looked for better ways of getting a string out of a net::IP object would it be in the net::IP class, or tucked somewhere else safe? On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/common/protobuf_utils.cpp, lines 202-203 https://reviews.apache.org/r/34687/diff/1/?file=972330#file972330line202 Could we do this error message inline in the return Error(...)? We can use stringify() to get the JSON string: return Error(Missing ... + stringify(json)); cool! will do, thanks. On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/tests/common/protobuf_utils_tests.cpp, line 23 https://reviews.apache.org/r/34687/diff/1/?file=972331#file972331line23 This (and standard c includes) goes first haven't we changed this recently? (I was assuming that the protobuf_utils.hpp was the 'relevant' header?) On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/tests/common/protobuf_utils_tests.cpp, line 61 https://reviews.apache.org/r/34687/diff/1/?file=972331#file972331line61 Should we encode this in human readable form? we probably could, but (a) the code would be pretty gnarly and (b) what's the point, really? this is what that IP string gets turned into when it becomes an int32 - we want to make sure this invariant does not get broken by future changes/refactorings. On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote: src/tests/common/protobuf_utils_tests.cpp, line 148 https://reviews.apache.org/r/34687/diff/1/?file=972331#file972331line148 What does 'FullCycle' mean? Mind added test descriptions as preceeding comments to the test blocks? eh eh I honestly spent five minutes trying to come up with something meaningful: epic fail, clearly! it actually means we go from UPID to MasterInfo to JSON and back to MasterInfo, and we want to get the same thing we started from. I thought the comment made that a bit clearer, but will elaborate on the point. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review85311 --- On May 26, 2015, 11:54 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/ --- (Updated May 26, 2015, 11:54 p.m.) Review request for mesos, haosdent huang and Niklas Nielsen. Bugs: MESOS-2340 https://issues.apache.org/jira/browse/MESOS-2340 Repository: mesos Description --- Jira: MESOS-2340 This is a preliminary step to enabling JSON API for Master Discovery via Zookeeper. We currently save the MasterInfo PB to ZK serializing directly the binary data, so that for clients to retrieve that information, they need to either link up with libmesos or obtain the PB definition (in mesos/mesos.proto). This change only provides the (de)serialization utility methods and associated tests. Diffs - src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b src/tests/common/protobuf_utils_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34687/diff/ Testing --- make check Thanks, Marco Massenzio
