Re: Review Request 34392: Added a method to Path which returns the modification time of the represented path.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34392/ --- (Updated June 15, 2015, 1:34 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Changes --- Till's comments. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- See summary. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c Diff: https://reviews.apache.org/r/34392/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35354: Smaller fixes in libprocess firewall initialization
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35354/ --- (Updated June 15, 2015, 11:24 a.m.) Review request for mesos, Ben Mahler and Till Toenshoff. Changes --- till's comments. Repository: mesos Description --- See summary. Diffs (updated) - src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294 src/messages/flags.hpp 41be419ba5593a600aa0c6c411210fa4faa829a8 src/slave/main.cpp c379243e01919a5ab30bb9dea1b738665ba4e746 Diff: https://reviews.apache.org/r/35354/diff/ Testing --- Manual checks make check Thanks, Alexander Rojas
Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35438/ --- (Updated June 15, 2015, 1:51 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen. Changes --- Added TODO regarding the 15 seconds naked constant when awaiting offers. Repository: mesos Description --- Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, fixed only one of two problems. Using DeclineOffers() instead of Return() should make the master resend offers so we can launch tasks. See line 205 below. Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we return a Try instead and follow call sites with EXPECT_SOME(task(s)). Diffs (updated) - src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 Diff: https://reviews.apache.org/r/35438/diff/ Testing --- make check Thanks, Bernd Mathiske
Re: Review Request 35353: Smaller fixes on libprocess firewall
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35353/ --- (Updated June 15, 2015, 11:24 a.m.) Review request for mesos, Ben Mahler and Till Toenshoff. Changes --- till's comments. Repository: mesos Description --- See summary Diffs (updated) - 3rdparty/libprocess/include/process/firewall.hpp 16ed852d07344e53c6f9d30d4cd7d99e07c6606d 3rdparty/libprocess/include/process/process.hpp 6a0b21d67912a40e0ec3220fdb250930be1979b2 3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/35353/diff/ Testing --- make check Thanks, Alexander Rojas
Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- Review request for mesos, Benjamin Hindman and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/#review87916 --- Patch looks great! Reviews applied: [35455] All tests passed. - Mesos ReviewBot On June 15, 2015, 12:20 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:20 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/#review87915 --- Ship it! docs/getting-started.md https://reviews.apache.org/r/35455/#comment140328 Not sure this makes sense for this chapter. docs/getting-started.md https://reviews.apache.org/r/35455/#comment140330 Also not yours but this does not seem to make any sense. OS X does not need Python3 to get installed just like the other distros dont need it. AFAIK we are fine with Python 2.7.x on all platforms. Could you please get rid of this part? docs/getting-started.md https://reviews.apache.org/r/35455/#comment140329 I know this isn't yours but it seems weird to mark C++, Java and Python as being code keywords, no? - Till Toenshoff On June 15, 2015, 12:20 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:20 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:11 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Changes --- Fixed copy-paste error. Repository: mesos Description --- See summary. Diffs (updated) - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:58 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Changes --- `s/#/$/` Repository: mesos Description --- See summary. Diffs (updated) - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Re: Review Request 34392: Added a method to Path which returns the modification time of the represented path.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34392/#review87919 --- Patch looks great! Reviews applied: [34392] All tests passed. - Mesos ReviewBot On June 15, 2015, 11:34 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34392/ --- (Updated June 15, 2015, 11:34 a.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 --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c Diff: https://reviews.apache.org/r/34392/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:42 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Changes --- Since we don't download the `maven` binary and un-`tar` it, `tar` is no longer a utility package we require. Repository: mesos Description --- See summary. Diffs (updated) - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Re: Review Request 35433: CHECK that checkpointed resources exist on the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 15, 2015, 12:39 p.m.) Review request for mesos, Benjamin Hindman and Jie Yu. Changes --- Addressed BenH's comment. The slave now sends `TASK_LOST` status updates since the fact that the checkpointed resources are available on the slave is not a global invariant we try to guarantee. Repository: mesos Description --- No bug was observed (yet), but realized I forgot about this in the dynamic reservations patches. Diffs (updated) - src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c Diff: https://reviews.apache.org/r/35433/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 35433: CHECK that checkpointed resources exist on the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review87918 --- src/slave/slave.cpp https://reviews.apache.org/r/35433/#comment140336 Introduce a new `REASON_` enum for this. src/slave/slave.cpp https://reviews.apache.org/r/35433/#comment140337 Same here - Michael Park On June 15, 2015, 12:39 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 15, 2015, 12:39 p.m.) Review request for mesos, Benjamin Hindman and Jie Yu. Repository: mesos Description --- No bug was observed (yet), but realized I forgot about this in the dynamic reservations patches. Diffs - src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c Diff: https://reviews.apache.org/r/35433/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 35433: CHECK that checkpointed resources exist on the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review87920 --- Patch looks great! Reviews applied: [35433] All tests passed. - Mesos ReviewBot On June 15, 2015, 12:39 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 15, 2015, 12:39 p.m.) Review request for mesos, Benjamin Hindman and Jie Yu. Repository: mesos Description --- No bug was observed (yet), but realized I forgot about this in the dynamic reservations patches. Diffs - src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c Diff: https://reviews.apache.org/r/35433/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:36 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Changes --- Addressed Till's comments. Repository: mesos Description --- See summary. Diffs (updated) - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:54 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Changes --- Add `tar` back, since we actually still need it. Repository: mesos Description --- See summary. Diffs (updated) - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
Review Request 35463: Fix typos in stout README.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35463/ --- Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/README.md 6fc09d6d3cc80c7155a6edc76467c765b160a465 Diff: https://reviews.apache.org/r/35463/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 3:25 p.m.) Review request for mesos, Adam B and Cody Maloney. Changes --- Address Adam's review comments. Bugs: MESOS-1733 https://issues.apache.org/jira/browse/MESOS-1733 Repository: mesos Description --- This change takes an un-complicated/naive route ( no trimming of values etc ) at making path::join(...) variadic mainly in order to preserve the earlier over-loaded join functionality. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35438/#review87928 --- Patch looks great! Reviews applied: [35438] All tests passed. - Mesos ReviewBot On June 15, 2015, 2:28 p.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35438/ --- (Updated June 15, 2015, 2:28 p.m.) Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, fixed only one of two problems. Using DeclineOffers() instead of Return() should make the master resend offers so we can launch tasks. See line 205 below. Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we return a Try instead and follow call sites with EXPECT_SOME(task(s)). Diffs - src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 Diff: https://reviews.apache.org/r/35438/diff/ Testing --- make check Thanks, Bernd Mathiske
Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35438/ --- (Updated June 15, 2015, 7:28 a.m.) Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and Vinod Kone. Changes --- Addressed Till's review. Repository: mesos Description --- Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, fixed only one of two problems. Using DeclineOffers() instead of Return() should make the master resend offers so we can launch tasks. See line 205 below. Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we return a Try instead and follow call sites with EXPECT_SOME(task(s)). Diffs (updated) - src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 Diff: https://reviews.apache.org/r/35438/diff/ Testing --- make check Thanks, Bernd Mathiske
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 June 15, 2015, 5:26 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- mpark's comments. Changes serialization from UTC to GMT to get around a bug in osx (see man strptime) 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 (updated) - 3rdparty/libprocess/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35455: Update 'Getting Started' documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/#review87922 --- Patch looks great! Reviews applied: [35455] All tests passed. - Mesos ReviewBot On June 15, 2015, 12:58 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35455/ --- (Updated June 15, 2015, 12:58 p.m.) Review request for mesos, Benjamin Hindman and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 Diff: https://reviews.apache.org/r/35455/diff/ Testing --- Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` and `CentOS 7.1` docker images. Thanks, Michael Park
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 June 15, 2015, 5:27 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Changes --- User `Time::create` to create time objects. Removes unnecessary code. 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 e47cc7afbc8110759edf25a2dc05d09eda25c417 3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35353: Smaller fixes on libprocess firewall
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35353/ --- (Updated June 15, 2015, 5:51 p.m.) Review request for mesos, Ben Mahler and Till Toenshoff. Changes --- Removes unused headers. Repository: mesos Description --- See summary Diffs (updated) - 3rdparty/libprocess/include/process/firewall.hpp 16ed852d07344e53c6f9d30d4cd7d99e07c6606d 3rdparty/libprocess/include/process/process.hpp 6a0b21d67912a40e0ec3220fdb250930be1979b2 3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/35353/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34427: AppC provisioner backend using bind mounts.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34427/#review88020 --- src/slave/containerizer/provisioners/appc/bind_backend.hpp https://reviews.apache.org/r/34427/#comment140424 Given that you dont declare any templates, would it make sense to split this into cpp+hpp? src/slave/containerizer/provisioners/appc/bind_backend.hpp https://reviews.apache.org/r/34427/#comment140425 - Till Toenshoff On May 19, 2015, 6:46 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34427/ --- (Updated May 19, 2015, 6:46 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Note: This is a specialized backend; see notes in bind_backend.hpp. Reproduced here for your convenience: This is a specialized backend that may be useful for deployments using large (multi-GB) single-layer images *and* where more recent kernel features such as overlayfs are not available. For small images (10's to 100's of MB) the Copy backend may be sufficient. 1) It supports only a single layer. Multi-layer images will fail to provision and the container will fail to launch! 2) The filesystem is read-only because all containers using this image share the source. Select writable areas can be achieved by mounting read-write volumes to places like /tmp, /var/tmp, /home, etc. using the ContainerInfo. These can be relative to the executor work directory. 3) It relies on the image persisting in the store. 4) It's fast because the bind mount requires (nearly) zero IO. Diffs - src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION src/slave/containerizer/provisioners/appc/bind_backend.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34427/diff/ Testing --- manual testing of a single layer image with RW relative bind mount for /tmp. Thanks, Ian Downes
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/#review87939 --- Patch looks great! Reviews applied: [34703, 30032] All tests passed. - Mesos ReviewBot On June 15, 2015, 3:27 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated June 15, 2015, 3:27 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417 3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 3rdparty/libprocess/src/tests/process_tests.cpp 660af45e7fd45bdf5d43ad9aa54477fd94f87058 Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 35410: Minor cleanups to the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35410/#review87943 --- Ship it! Ship It! - Jie Yu On June 13, 2015, 2:04 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35410/ --- (Updated June 13, 2015, 2:04 a.m.) Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos Description --- Minor cleanups to the slave. Diffs - src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 Diff: https://reviews.apache.org/r/35410/diff/ Testing --- Thanks, Ben Mahler
Re: Review Request 35470: Fixed the flaky oversbuscription tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35470/#review87961 --- Ship it! Ship It! - Ben Mahler On June 15, 2015, 7:02 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35470/ --- (Updated June 15, 2015, 7:02 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2869 https://issues.apache.org/jira/browse/MESOS-2869 Repository: mesos Description --- Fixed the flaky oversbuscription tests. Diffs - src/tests/oversubscription_tests.cpp e7d94cecb4a668f634e94fb0aba95155dc827510 Diff: https://reviews.apache.org/r/35470/diff/ Testing --- make check repeat the oversubscription tests for 100 times Thanks, Jie Yu
Review Request 35470: Fixed the flaky oversbuscription tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35470/ --- Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2869 https://issues.apache.org/jira/browse/MESOS-2869 Repository: mesos Description --- Fixed the flaky oversbuscription tests. Diffs - src/tests/oversubscription_tests.cpp e7d94cecb4a668f634e94fb0aba95155dc827510 Diff: https://reviews.apache.org/r/35470/diff/ Testing --- make check repeat the oversubscription tests for 100 times Thanks, Jie Yu
Re: Review Request 35411: Send oversubscribable resources during (re-)registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/#review87959 --- Ship it! src/slave/slave.cpp https://reviews.apache.org/r/35411/#comment140371 s/oversubscribable/oversubscribed/ src/slave/slave.cpp https://reviews.apache.org/r/35411/#comment140372 s/oversubscribable/oversubscribed/ - Vinod Kone On June 15, 2015, 5:59 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/ --- (Updated June 15, 2015, 5:59 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2866 https://issues.apache.org/jira/browse/MESOS-2866 Repository: mesos Description --- Send oversubscribable resources during (re-)registration. Diffs - src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca Diff: https://reviews.apache.org/r/35411/diff/ Testing --- Thanks, Ben Mahler
Re: Review Request 34361: converted hard-coded strings to consts
On June 9, 2015, 6:25 p.m., Ben Mahler wrote: src/tests/master_tests.cpp, lines 3031-3034 https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031 Why bother with all this? Why not just have `key1`, `value1`, `key2`, `value2` inlined appropriately throughout these tests. Very straightforward to read. Colin Williams wrote: I think the issue with the changes remaining is that the test depends on the same value occurring in several places. By consolidating to a variable it's no longer possible for these values to get out of sync. Niklas Nielsen wrote: Colin: exactly Ben: We have label tests three places; in the master, slave and in the modules and they use the same foo, bar, baz, qux key value pairs. The idea was to centralize them, so they don't get out of date and to avoid code duplication. Does that make sense? Ben Mahler wrote: Well, then let's just keep it simple and get rid of these special strings by just making the tests use key1, value1, key2, value2, etc. I'm not following your code duplication point, this introduces more code (now there are additional global constants, which we like to avoid if unnecessary). Also, each test is ideally independent, why does the master's test for labels affect the slave's test for labels? Let's say I need a test with 5 labels, you want to have 5x2=10 global constants to address this? Niklas Nielsen wrote: Try to see how testLabelKey and testLabelValue was used previously and foo, bar, qux was used on others and I created this ticket to unify the way we do this, along with seeing these key value pair being created in the slave and master tests. Dispite the current patch, I do think we can simply and unify the test label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious which key pairs are being tested. The comments in the test code need to carry a bunch of context, to make sense out of the label transformations (especially across the library border for the hooks tests). This is all in spirit of reducing the tech debt we introduced. We are on the same page on not introducing more lines/key pairs than before. Let us iterate on this and get back to you. Colin Williams wrote: Ben: I'm more in agreement with your sentiment, I'm not sure I see the point of unifying label names into a centralized variable that aren't at all related. Niklas: I was a little confused by the original task description, so I thought a sample patch would be a good discussion point. I don't entirely understand the tech debt you're trying to solve. For example, it seems like you're asking to have some constants defined somewhere that are used by both master_test and slave_test, but the the similarities between these two only seem incidental. I'm concerned that creating something like CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code. Niklas Nielsen wrote: Okay - thinking about this; I am on board with key1, value1 etc. Colin: the tech debt is that we have some inlined constants (like foo, bar, etc) and some which are constants (which have to be kept in sync between hooks modules and test body). The idea was to unify the way we name the key value pairs. Do you want to update this ticket to reflect this, or come with a new patch set which tackles streaming the two? Ping :) - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/#review87331 --- On June 8, 2015, 12:05 p.m., Colin Williams wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/ --- (Updated June 8, 2015, 12:05 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2637 https://issues.apache.org/jira/browse/MESOS-2637 Repository: mesos Description --- converted hard-coded strings to consts Diffs - src/tests/hook_tests.cpp 3ffde6d src/tests/master_tests.cpp ba3858f src/tests/slave_tests.cpp acae497 Diff: https://reviews.apache.org/r/34361/diff/ Testing --- Thanks, Colin Williams
Re: Review Request 35410: Minor cleanups to the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35410/#review87956 --- Ship it! Ship It! - Vinod Kone On June 13, 2015, 2:04 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35410/ --- (Updated June 13, 2015, 2:04 a.m.) Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos Description --- Minor cleanups to the slave. Diffs - src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 Diff: https://reviews.apache.org/r/35410/diff/ Testing --- Thanks, Ben Mahler
Re: Review Request 33208: Delete detector in MesosSchedulerDriver::stop
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33208/#review87958 --- Hey Robert; BenH helped out and wrote a PoC patch here https://reviews.apache.org/r/35405 In short; it is not safe to delete the detector at this point. The patch above does it in join() and has a good descriptive block of comment explaining the issue. What we need to do is try it out and test it. - Niklas Nielsen On April 14, 2015, 10:23 p.m., Robert Lacroix wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33208/ --- (Updated April 14, 2015, 10:23 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-1634 https://issues.apache.org/jira/browse/MESOS-1634 Repository: mesos Description --- When the Mesos Scheduler Driver stops, it does not delete the detector process before the object is garbage collected, which leaves ZooKeeper connections hanging around unnecessarily. This deletes the process on stop as well, not only on destruction. Diffs - src/sched/sched.cpp 66fd2b3146568900356cc48e0f17c6720665ae80 Diff: https://reviews.apache.org/r/33208/diff/ Testing --- make check, manual testing Thanks, Robert Lacroix
Re: Review Request 35411: Send oversubscribable resources during (re-)registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/#review87944 --- Ship it! Ship It! - Jie Yu On June 13, 2015, 2:04 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/ --- (Updated June 13, 2015, 2:04 a.m.) Review request for mesos, Jie Yu and Vinod Kone. Bugs: MESOS-2866 https://issues.apache.org/jira/browse/MESOS-2866 Repository: mesos Description --- Send oversubscribable resources during (re-)registration. Diffs - src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca Diff: https://reviews.apache.org/r/35411/diff/ Testing --- Thanks, Ben Mahler
Re: Review Request 35467: Fix a comment in Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35467/#review87945 --- Patch looks great! Reviews applied: [35467] All tests passed. - Mesos ReviewBot On June 15, 2015, 5:37 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35467/ --- (Updated June 15, 2015, 5:37 p.m.) Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c Diff: https://reviews.apache.org/r/35467/diff/ Testing --- Comment fix. Thanks, Michael Park
Re: Review Request 34375: Removed use of namespace aliases.
On May 19, 2015, 2:02 a.m., Ben Mahler wrote: This might be a valid namespace alias use case that we hadn't considered, because there is no way to be able to write just `http::Response` otherwise, is there? Seems quite verbose to write process::http everywhere, and on the otherhand just having `Request` or `Response` seems to miss the context of it being http, thoughts? Michael Park wrote: This might be a valid namespace alias use case that we hadn't considered I actually think we should reconsider the overall approach of disallowing aliases (e.g. type, namespace). because there is no way to be able to write just http::Response otherwise, is there? We could do so by declaring `using namespace process;` if we wanted to. Seems quite verbose to write process::http everywhere and on the otherhand just having Request or Response seems to miss the context of it being http This is the entire point of aliases in general, which is why I think we should reconsider the decision to disallow them. thoughts? From a offline discussion with BenH, it seems like the decision to disallow type and namespace aliases to avoid inconsistencies in different files. For example, one person could write `namespace http = process::http;` and another person could write `namespace phttp = process::http;`. I think the correct solution is to come up with naming guidelines to give them good and consistent names, rather than disallowing the feature. For example, we don't disallow functions just because someone could possibly give it a horrible name. We instead try to influence people to give their functions meaningful names in a consistent manner. An example of such rule is that we write `member()` rather than `getMember()` for our getter functions. We don't simply disallow getter functions just because people could say them inconsistently. Anyway, given the current rules, I would push for us to be consistent with no special cases. Especially since we have much longer names all over in the codebase that we cope with. (e.g. `mesos::master::allocator::Allocator`, `master::allocator::MesosAllocatorProcess::deactivateFramework`, `master::allocator::HierarchicalDRFAllocator`) I'm definitely interested in starting the discussion around reconsidering the rules and perhaps formalizing some of the naming rules around aliases, but I also think that that should happen outside of this review, probably in a JIRA ticket. Sounds great Michael! Happy to help you get these aliases added, at least restricted to this use case initially. :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34375/#review84251 --- On June 13, 2015, 9:51 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34375/ --- (Updated June 13, 2015, 9:51 p.m.) Review request for mesos and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - src/master/master.cpp 3049e057c87571f5db73ee0b14db8b47132b2dff src/slave/slave.cpp 9af69d8f0b28c9441c684886c52320378f9b2869 src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b src/tests/slave_tests.cpp 8dae6e0842c2bedddc13d1c036390e85c7a96df7 Diff: https://reviews.apache.org/r/34375/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 34835: Add constexpr to C++ whitelist
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34835/#review87954 --- Patch looks great! Reviews applied: [34835] All tests passed. - Mesos ReviewBot On June 15, 2015, 5:59 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34835/ --- (Updated June 15, 2015, 5:59 p.m.) Review request for mesos, Benjamin Hindman, Dave Lester, Ian Downes, and Vinod Kone. Bugs: MESOS-2784 https://issues.apache.org/jira/browse/MESOS-2784 Repository: mesos Description --- Add constexpr to C++11 whitelist Diffs - docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e Diff: https://reviews.apache.org/r/34835/diff/ Testing --- Thanks, Paul Brett
Re: Review Request 34361: converted hard-coded strings to consts
On June 10, 2015, 1:25 a.m., Ben Mahler wrote: src/tests/master_tests.cpp, lines 3031-3034 https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031 Why bother with all this? Why not just have `key1`, `value1`, `key2`, `value2` inlined appropriately throughout these tests. Very straightforward to read. Colin Williams wrote: I think the issue with the changes remaining is that the test depends on the same value occurring in several places. By consolidating to a variable it's no longer possible for these values to get out of sync. Niklas Nielsen wrote: Colin: exactly Ben: We have label tests three places; in the master, slave and in the modules and they use the same foo, bar, baz, qux key value pairs. The idea was to centralize them, so they don't get out of date and to avoid code duplication. Does that make sense? Ben Mahler wrote: Well, then let's just keep it simple and get rid of these special strings by just making the tests use key1, value1, key2, value2, etc. I'm not following your code duplication point, this introduces more code (now there are additional global constants, which we like to avoid if unnecessary). Also, each test is ideally independent, why does the master's test for labels affect the slave's test for labels? Let's say I need a test with 5 labels, you want to have 5x2=10 global constants to address this? Niklas Nielsen wrote: Try to see how testLabelKey and testLabelValue was used previously and foo, bar, qux was used on others and I created this ticket to unify the way we do this, along with seeing these key value pair being created in the slave and master tests. Dispite the current patch, I do think we can simply and unify the test label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious which key pairs are being tested. The comments in the test code need to carry a bunch of context, to make sense out of the label transformations (especially across the library border for the hooks tests). This is all in spirit of reducing the tech debt we introduced. We are on the same page on not introducing more lines/key pairs than before. Let us iterate on this and get back to you. Colin Williams wrote: Ben: I'm more in agreement with your sentiment, I'm not sure I see the point of unifying label names into a centralized variable that aren't at all related. Niklas: I was a little confused by the original task description, so I thought a sample patch would be a good discussion point. I don't entirely understand the tech debt you're trying to solve. For example, it seems like you're asking to have some constants defined somewhere that are used by both master_test and slave_test, but the the similarities between these two only seem incidental. I'm concerned that creating something like CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code. Niklas Nielsen wrote: Okay - thinking about this; I am on board with key1, value1 etc. Colin: the tech debt is that we have some inlined constants (like foo, bar, etc) and some which are constants (which have to be kept in sync between hooks modules and test body). The idea was to unify the way we name the key value pairs. Do you want to update this ticket to reflect this, or come with a new patch set which tackles streaming the two? Niklas Nielsen wrote: Ping :) Sorry, my day job's been really all-consuming the last few days. I was planning to update the ticket. - Colin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/#review87331 --- On June 8, 2015, 7:05 p.m., Colin Williams wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/ --- (Updated June 8, 2015, 7:05 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2637 https://issues.apache.org/jira/browse/MESOS-2637 Repository: mesos Description --- converted hard-coded strings to consts Diffs - src/tests/hook_tests.cpp 3ffde6d src/tests/master_tests.cpp ba3858f src/tests/slave_tests.cpp acae497 Diff: https://reviews.apache.org/r/34361/diff/ Testing --- Thanks, Colin Williams
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87933 --- Patch looks great! Reviews applied: [35179] All tests passed. - Mesos ReviewBot On June 15, 2015, 3:25 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 3:25 p.m.) Review request for mesos, Adam B and Cody Maloney. Bugs: MESOS-1733 https://issues.apache.org/jira/browse/MESOS-1733 Repository: mesos Description --- This change takes an un-complicated/naive route ( no trimming of values etc ) at making path::join(...) variadic mainly in order to preserve the earlier over-loaded join functionality. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35411: Send oversubscribable resources during (re-)registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/#review87968 --- Just wanted to tag on this review and follow the update changes - LGTM :) src/tests/oversubscription_tests.cpp https://reviews.apache.org/r/35411/#comment140373 Which kind of update message? :) s/update/'SlaveUpdate'/ ? src/tests/oversubscription_tests.cpp https://reviews.apache.org/r/35411/#comment140375 You have the PIDs of the master and slave - would it make sense to be explicit in the pattern matching? src/tests/oversubscription_tests.cpp https://reviews.apache.org/r/35411/#comment140374 Should we const 'resources'? - Niklas Nielsen On June 15, 2015, 10:59 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/ --- (Updated June 15, 2015, 10:59 a.m.) Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2866 https://issues.apache.org/jira/browse/MESOS-2866 Repository: mesos Description --- Send oversubscribable resources during (re-)registration. Diffs - src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca Diff: https://reviews.apache.org/r/35411/diff/ Testing --- Thanks, Ben Mahler
Re: Review Request 35470: Fixed the flaky oversbuscription tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35470/#review87965 --- Patch looks great! Reviews applied: [35470] All tests passed. - Mesos ReviewBot On June 15, 2015, 7:02 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35470/ --- (Updated June 15, 2015, 7:02 p.m.) Review request for mesos, Niklas Nielsen and Vinod Kone. Bugs: MESOS-2869 https://issues.apache.org/jira/browse/MESOS-2869 Repository: mesos Description --- Fixed the flaky oversbuscription tests. Diffs - src/tests/oversubscription_tests.cpp e7d94cecb4a668f634e94fb0aba95155dc827510 Diff: https://reviews.apache.org/r/35470/diff/ Testing --- make check repeat the oversubscription tests for 100 times Thanks, Jie Yu
Re: Review Request 35441: Removed unused macros.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35441/#review87966 --- Ship it! Ship It! - Niklas Nielsen On June 14, 2015, 10:26 a.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35441/ --- (Updated June 14, 2015, 10:26 a.m.) Review request for mesos and Niklas Nielsen. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76 Diff: https://reviews.apache.org/r/35441/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35473/#review87982 --- Patch looks great! Reviews applied: [35473] All tests passed. - Mesos ReviewBot On June 15, 2015, 10:03 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35473/ --- (Updated June 15, 2015, 10:03 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2627 https://issues.apache.org/jira/browse/MESOS-2627 Repository: mesos Description --- Removed a few incorrect CHECKs in DRF sorter. See details in the ticket for the reason why this is needed. Diffs - src/master/allocator/sorter/drf/sorter.cpp ac05afdc7d408735dd796faa68c943e75540aaa7 src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 Diff: https://reviews.apache.org/r/35473/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35473/#review87980
---
Ship it!
I personally find it easier to read if we don't have the special 'return' cases:
```
{
if (!resources.empty()) {
// Do work.
}
}
Seems clearer than:
{
if (resources.empty()) {
// Don't do work by returning.
}
// Do work.
}
```
I think most of the cases where we've done the early returns were motivated by
a few reasons that don't hold here:
(1) We wanted to avoid deep nesting of logic (using 'returns' allowed us to
flatten the logical structure), and/or
(2) We wanted to eliminate many special cases in sequence, until only the
correct case remains (e.g. message handlers, (a) can't lookup framework, (b)
message sent by wrong pid, (c) invalid offers used, ...).
Up to you!
src/tests/examples_tests.cpp
https://reviews.apache.org/r/35473/#comment140390
Don't forget to delete this
- Ben Mahler
On June 15, 2015, 10:03 p.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35473/
---
(Updated June 15, 2015, 10:03 p.m.)
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2627
https://issues.apache.org/jira/browse/MESOS-2627
Repository: mesos
Description
---
Removed a few incorrect CHECKs in DRF sorter.
See details in the ticket for the reason why this is needed.
Diffs
-
src/master/allocator/sorter/drf/sorter.cpp
ac05afdc7d408735dd796faa68c943e75540aaa7
src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17
Diff: https://reviews.apache.org/r/35473/diff/
Testing
---
sudo make check
Thanks,
Jie Yu
Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35473/ --- Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2627 https://issues.apache.org/jira/browse/MESOS-2627 Repository: mesos Description --- Removed a few incorrect CHECKs in DRF sorter. See details in the ticket for the reason why this is needed. Diffs - src/master/allocator/sorter/drf/sorter.cpp ac05afdc7d408735dd796faa68c943e75540aaa7 Diff: https://reviews.apache.org/r/35473/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 35411: Send oversubscribable resources during (re-)registration.
On June 15, 2015, 8:21 p.m., Niklas Nielsen wrote: src/tests/oversubscription_tests.cpp, line 498 https://reviews.apache.org/r/35411/diff/1/?file=983985#file983985line498 Which kind of update message? :) s/update/'SlaveUpdate'/ ? Changed it to forwards the estimation to match the other test comments, thanks! On June 15, 2015, 8:21 p.m., Niklas Nielsen wrote: src/tests/oversubscription_tests.cpp, line 528 https://reviews.apache.org/r/35411/diff/1/?file=983985#file983985line528 You have the PIDs of the master and slave - would it make sense to be explicit in the pattern matching? Since none of the tests in this file use the PID matching, I followed suit for consistency. It would be nice to use them in general, but there are cases where we don't have both PIDs yet (e.g. need to set up the expectation before starting the slave). On June 15, 2015, 8:21 p.m., Niklas Nielsen wrote: src/tests/oversubscription_tests.cpp, line 538 https://reviews.apache.org/r/35411/diff/1/?file=983985#file983985line538 Should we const 'resources'? Hm.. seems inconsistent with the rest of this file, but also, do we want to proliferate 'const' everywhere? There are a ton of scope variables in this file that can be const but are not, so I'm not concerned about it. I haven't really noticed a lot of benefit from having scope variables as 'const' where not necessary, have you? Would love to see some examples as the code is getting a bit inconsistent with 'const' usage. We have a ton of variables that can be const, but adding const to all of these might be too verbose IMO :( - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/#review87968 --- On June 15, 2015, 5:59 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35411/ --- (Updated June 15, 2015, 5:59 p.m.) Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2866 https://issues.apache.org/jira/browse/MESOS-2866 Repository: mesos Description --- Send oversubscribable resources during (re-)registration. Diffs - src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 src/tests/oversubscription_tests.cpp e8ae053dd9cd712e49bd2830e414b7a3d99c20ca Diff: https://reviews.apache.org/r/35411/diff/ Testing --- Thanks, Ben Mahler
Re: Review Request 35441: Removed unused macros.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35441/#review87972 --- Ship it! Ship It! - Michael Park On June 14, 2015, 5:26 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35441/ --- (Updated June 14, 2015, 5:26 p.m.) Review request for mesos and Niklas Nielsen. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76 Diff: https://reviews.apache.org/r/35441/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 30339: Call hookmanager only if some hooks are installed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/ --- (Updated June 15, 2015, 5:22 p.m.) Review request for mesos, Niklas Nielsen and Till Toenshoff. Changes --- Added mutex check Repository: mesos Description --- Call hook manager only if hooks were specified on the commandline. Diffs (updated) - src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955 src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0 src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05 src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1 Diff: https://reviews.apache.org/r/30339/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 30339: Call hookmanager only if some hooks are installed.
On June 3, 2015, 1:17 p.m., Niklas Nielsen wrote: src/hook/manager.cpp, line 95 https://reviews.apache.org/r/30339/diff/3/?file=975940#file975940line95 Don't you need to acquire the mutex here? Good catch. Fixed. - Kapil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/#review86436 --- On June 15, 2015, 5:22 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/ --- (Updated June 15, 2015, 5:22 p.m.) Review request for mesos, Niklas Nielsen and Till Toenshoff. Repository: mesos Description --- Call hook manager only if hooks were specified on the commandline. Diffs - src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955 src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0 src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05 src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1 Diff: https://reviews.apache.org/r/30339/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.
On June 15, 2015, 9:40 p.m., Ben Mahler wrote:
src/master/allocator/sorter/drf/sorter.cpp, lines 176-188
https://reviews.apache.org/r/35473/diff/1/?file=984846#file984846line176
How about the same logic in both?
```
{
if (_resources.empty()) {
// Do work.
}
}
```
In the case of remove(), we can have a CHECK that the slave id is
contained in the map as part of the work?
Sorry:
```
{
if (!_resources.empty()) {
// Do work.
}
}
```
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35473/#review87975
---
On June 15, 2015, 9:15 p.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35473/
---
(Updated June 15, 2015, 9:15 p.m.)
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2627
https://issues.apache.org/jira/browse/MESOS-2627
Repository: mesos
Description
---
Removed a few incorrect CHECKs in DRF sorter.
See details in the ticket for the reason why this is needed.
Diffs
-
src/master/allocator/sorter/drf/sorter.cpp
ac05afdc7d408735dd796faa68c943e75540aaa7
Diff: https://reviews.apache.org/r/35473/diff/
Testing
---
sudo make check
Thanks,
Jie Yu
Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35473/ --- (Updated June 15, 2015, 10:03 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Changes --- Review comments. Bugs: MESOS-2627 https://issues.apache.org/jira/browse/MESOS-2627 Repository: mesos Description --- Removed a few incorrect CHECKs in DRF sorter. See details in the ticket for the reason why this is needed. Diffs (updated) - src/master/allocator/sorter/drf/sorter.cpp ac05afdc7d408735dd796faa68c943e75540aaa7 src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 Diff: https://reviews.apache.org/r/35473/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 35441: Removed unused macros.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35441/#review87995 --- Ship it! Ship It! - Alexander Rojas On June 14, 2015, 7:26 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35441/ --- (Updated June 14, 2015, 7:26 p.m.) Review request for mesos and Niklas Nielsen. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 95b4b33b70c37640d97dff5d5724550d42048b76 Diff: https://reviews.apache.org/r/35441/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87999 --- Patch looks great! Reviews applied: [35179] All tests passed. - Mesos ReviewBot On June 15, 2015, 11:23 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 11:23 p.m.) Review request for mesos, Adam B and Cody Maloney. Bugs: MESOS-1733 https://issues.apache.org/jira/browse/MESOS-1733 Repository: mesos Description --- This change takes an un-complicated/naive route ( no trimming of values etc ) at making path::join(...) variadic mainly in order to preserve the earlier over-loaded join functionality. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35438/#review87895 --- src/tests/fetcher_cache_tests.cpp https://reviews.apache.org/r/35438/#comment140309 Does this mean when the default changes we need to change this value too? Also what's the rationale making it a Error/Try? Is this because a timeout can be a valid case here? - Timothy Chen On June 14, 2015, 1:54 p.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35438/ --- (Updated June 14, 2015, 1:54 p.m.) Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, fixed only one of two problems. Using DeclineOffers() instead of Return() should make the master resend offers so we can launch tasks. See line 205 below. Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we return a Try instead and follow call sites with EXPECT_SOME(task(s)). Diffs - src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 Diff: https://reviews.apache.org/r/35438/diff/ Testing --- make check Thanks, Bernd Mathiske
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87989 --- 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140398 Maybe `s/Default/Base/`? 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140400 Not yours, but could you fix the formatting here to not wrap after `return` please? ``` return strings::remove(path1, /, strings::SUFFIX) + / + strings::remove(path2, /, strings::PREFIX); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140401 Not yours, but could you fix the formatting here to not wrap after `return` please? ``` return strings::remove(path1, /, strings::SUFFIX) + / + strings::remove(path2, /, strings::PREFIX); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140399 `s/path1 ,/path1,` - Michael Park On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 10:45 p.m.) Review request for mesos, Adam B and Cody Maloney. Bugs: MESOS-1733 https://issues.apache.org/jira/browse/MESOS-1733 Repository: mesos Description --- This change takes an un-complicated/naive route ( no trimming of values etc ) at making path::join(...) variadic mainly in order to preserve the earlier over-loaded join functionality. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 29406: Introduce libevent ssl socket.
On June 15, 2015, 5:28 p.m., Jojy Varghese wrote: 3rdparty/libprocess/include/process/address.hpp, line 84 https://reviews.apache.org/r/29406/diff/29/?file=984330#file984330line84 There are a few if family == INET (or similar) in the code. By specializing the net address structures on FAMILY, we will get rid of them. As most of the network structures (socket for example) are classified based on family, this should naturally fit into the overall scheme. I think we'll probably want a version-agnostic one as well as specialized ones if we care about supporting the differences IPv4 and IPv6. `Boost.Asio` and the proposed networking library for the C++ standard based on it provide `address`, `address_v4` and `address_v6` for example. We could provide the same alternatives via template specializations, but my point is that we'll probably want to keep the version-agnostic one for cases where we can't know or don't care which version we have. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29406/#review87936 --- On June 13, 2015, 9:23 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29406/ --- (Updated June 13, 2015, 9:23 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-1913 https://issues.apache.org/jira/browse/MESOS-1913 Repository: mesos Description --- Requires: configure --enable-libevent --enable-libevent-socket --enable-ssl New environment variables: ``` SSL_ENABLED=(false|0,true|1) SSL_CERT_FILE=(path to certificate) SSL_KEY_FILE=(path to key) SSL_VERIFY_CERT=(false|0,true|1) SSL_REQUIRE_CERT=(false|0,true|1) SSL_VERIFY_DEPTH=(4) SSL_CA_DIR=(path to CA directory) SSL_CA_FILE=(path to CA file) SSL_CIPHERS=(accepted ciphers separated by ':') SSL_ENABLE_SSL_V2=(false|0,true|1) SSL_ENABLE_SSL_V3=(false|0,true|1) SSL_ENABLE_TLS_V1_0=(false|0,true|1) SSL_ENABLE_TLS_V1_1=(false|0,true|1) SSL_ENABLE_TLS_V1_2=(false|0,true|1) ``` Only TLSV1.2 is enabled by default. Use the `ENABLE_SSL_V*` and `ENABLE_TLS_V*` environment variables to open up more protocols. Use the `SSL_CIPHERS` environment variable to restrict or open up the supported ciphers. Diffs - 3rdparty/libprocess/Makefile.am 489ce359f383d819299335cbaa8c95724b0c6ac2 3rdparty/libprocess/include/process/address.hpp 729f5cd7ea981e43a33c1fe9d99d58b906a31158 3rdparty/libprocess/include/process/socket.hpp b8c2274de535ac473e49a09165b601c96d3ebe8b 3rdparty/libprocess/src/libevent.hpp f6cc72178613a30446629532a773afccfd404212 3rdparty/libprocess/src/libevent.cpp fb038597358135a06c1927d079cb7cb09fea7452 3rdparty/libprocess/src/libevent_ssl_socket.hpp PRE-CREATION 3rdparty/libprocess/src/libevent_ssl_socket.cpp PRE-CREATION 3rdparty/libprocess/src/openssl.hpp PRE-CREATION 3rdparty/libprocess/src/openssl.cpp PRE-CREATION 3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198 3rdparty/libprocess/src/socket.cpp 0e1cebb19e21c706b152d35a0b8722924c971a35 Diff: https://reviews.apache.org/r/29406/diff/ Testing --- make check (uses non-ssl socket) benchmarks using ssl sockets master, slave, framework, webui launch with ssl sockets Thanks, Joris Van Remoortere
Re: Review Request 30339: Call hookmanager only if some hooks are installed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/ --- (Updated June 15, 2015, 6:26 p.m.) Review request for mesos, Niklas Nielsen and Till Toenshoff. Changes --- Removed unneeded changes from hook tests. Repository: mesos Description --- Call hook manager only if hooks were specified on the commandline. Diffs (updated) - src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c Diff: https://reviews.apache.org/r/30339/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 35404: Fixed http::Request::client to be set correctly.
On June 12, 2015, 11:20 p.m., Jie Yu wrote: Have you tested this? Yep, the test was updated to properly test this, and I verified manually. :) On June 12, 2015, 11:20 p.m., Jie Yu wrote: 3rdparty/libprocess/src/process.cpp, line 593 https://reviews.apache.org/r/35404/diff/1/?file=983948#file983948line593 Not sure if this will cause a scalability issue since we are calling getpeername everytime there are some requests coming in. Maybe we should cache the peer address when the socket is accepted. Can you add a TODO? Added a TODO on Socket::peer which matches the existing one in Socket::address. FWIW I measure about 1 microsecond for this call, excluding timer overhead, so should be ok without the caching, but I'd love to avoid the syscall overhead here! :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35404/#review87781 --- On June 12, 2015, 10:40 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35404/ --- (Updated June 12, 2015, 10:40 p.m.) Review request for mesos and Jie Yu. Bugs: MESOS-2519 https://issues.apache.org/jira/browse/MESOS-2519 Repository: mesos Description --- This now uses `Socket::peer` to augment the `http::Request`. Diffs - 3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198 3rdparty/libprocess/src/tests/http_tests.cpp 8444b9c961d04b932188b2ac37e2a42aafda1abd Diff: https://reviews.apache.org/r/35404/diff/ Testing --- Updated the test. Thanks, Ben Mahler
Re: Review Request 35403: Added the ability to get the peer address of a connected or accepted Socket.
On June 12, 2015, 11:06 p.m., Niklas Nielsen wrote: Maybe add @joris to this review? This change is pretty trivial, it's symmetric to the existing Socket::address, but I'd be happy to follow up if he has any feedback! :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35403/#review87779 --- On June 12, 2015, 10:40 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35403/ --- (Updated June 12, 2015, 10:40 p.m.) Review request for mesos and Jie Yu. Repository: mesos Description --- See above. Diffs - 3rdparty/libprocess/include/process/network.hpp 6d949c0b9b5bfd2b54be76a08ab2b6970be5d4fa 3rdparty/libprocess/include/process/socket.hpp b8c2274de535ac473e49a09165b601c96d3ebe8b 3rdparty/libprocess/src/socket.cpp 0e1cebb19e21c706b152d35a0b8722924c971a35 Diff: https://reviews.apache.org/r/35403/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 35179: MESOS-1733 Variadic Path Join
On June 15, 2015, 10:57 p.m., Michael Park wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 49-51 https://reviews.apache.org/r/35179/diff/6/?file=984730#file984730line49 Not yours, but could you fix the formatting here to not wrap after `return` please? ``` return strings::remove(path1, /, strings::SUFFIX) + / + strings::remove(path2, /, strings::PREFIX); ``` Not sure why this got duplicated. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87989 --- On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 10:45 p.m.) Review request for mesos, Adam B and Cody Maloney. Bugs: MESOS-1733 https://issues.apache.org/jira/browse/MESOS-1733 Repository: mesos Description --- This change takes an un-complicated/naive route ( no trimming of values etc ) at making path::join(...) variadic mainly in order to preserve the earlier over-loaded join functionality. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 30339: Call hookmanager only if some hooks are installed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/#review87991 --- Patch looks great! Reviews applied: [30339] All tests passed. - Mesos ReviewBot On June 15, 2015, 10:26 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/ --- (Updated June 15, 2015, 10:26 p.m.) Review request for mesos, Niklas Nielsen and Till Toenshoff. Repository: mesos Description --- Call hook manager only if hooks were specified on the commandline. Diffs - src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c Diff: https://reviews.apache.org/r/30339/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 35129: Refactor Future::Data to use ResultT. Remove dynamic allocation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/#review88000 --- Ship it! Ship It! - Michael Park On June 14, 2015, 4:17 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/ --- (Updated June 14, 2015, 4:17 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-2801 https://issues.apache.org/jira/browse/MESOS-2801 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/future.hpp adfad6faf89a52bf2da90d10a29e3d34502898bd Diff: https://reviews.apache.org/r/35129/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 34943: Added validation to flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/#review87996 --- Very neat! LGTM - however, my gut feeling is that we should only include the signature comment once and reference it from the other call sites. Other than that, only a question about whether we should do CHECK_NOTNULL on the pointers you take in. 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp https://reviews.apache.org/r/34943/#comment140406 Should we have a CHECK_NOTNULL for this option? 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp https://reviews.apache.org/r/34943/#comment140409 I am a bit torn whether we should copy this block so many places (taken the number of times it needs to get updates/kept in sync). Should we reference the first block from the other places? - Niklas Nielsen On June 15, 2015, 10:52 a.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/ --- (Updated June 15, 2015, 10:52 a.m.) Review request for mesos and Niklas Nielsen. Repository: mesos Description --- Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to get rid of our loader and stringifier functors. Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984 Diff: https://reviews.apache.org/r/34943/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35129: Refactor Future::Data to use ResultT. Remove dynamic allocation.
On June 15, 2015, 11:47 p.m., Michael Park wrote: Ship It! Looks like you might have to rebase this one more time? - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/#review88000 --- On June 14, 2015, 4:17 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/ --- (Updated June 14, 2015, 4:17 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-2801 https://issues.apache.org/jira/browse/MESOS-2801 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/future.hpp adfad6faf89a52bf2da90d10a29e3d34502898bd Diff: https://reviews.apache.org/r/35129/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 35129: Refactor Future::Data to use ResultT. Remove dynamic allocation.
On June 15, 2015, 11:47 p.m., Michael Park wrote: Ship It! Michael Park wrote: Looks like you might have to rebase this one more time? Never mind, it applies just fine. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/#review88000 --- On June 14, 2015, 4:17 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35129/ --- (Updated June 14, 2015, 4:17 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-2801 https://issues.apache.org/jira/browse/MESOS-2801 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/future.hpp adfad6faf89a52bf2da90d10a29e3d34502898bd Diff: https://reviews.apache.org/r/35129/diff/ Testing --- make check Thanks, Joris Van Remoortere
Review Request 35482: Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35482/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter. Diffs - src/Makefile.am 3c44f01f7d22cff6b9e46c2e68997ce5c2773791 src/common/json.hpp PRE-CREATION src/common/json.cpp PRE-CREATION src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc src/slave/containerizer/isolators/network/port_mapping.cpp 432b05ce5a99c8239fafc47a6b65d46a0fbac26e src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 Diff: https://reviews.apache.org/r/35482/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 35482: Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35482/#review88008 --- Bad patch! Reviews applied: [35482] Failed command: ./support/apply-review.sh -n -r 35482 Error: 2015-06-16 00:15:18 URL:https://reviews.apache.org/r/35482/diff/raw/ [44012/44012] - 35482.patch [1] error: patch failed: src/Makefile.am:353 error: src/Makefile.am: patch does not apply Failed to apply patch - Mesos ReviewBot On June 16, 2015, 12:14 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35482/ --- (Updated June 16, 2015, 12:14 a.m.) Review request for mesos and Jie Yu. Repository: mesos Description --- Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter. Diffs - src/Makefile.am 3c44f01f7d22cff6b9e46c2e68997ce5c2773791 src/common/json.hpp PRE-CREATION src/common/json.cpp PRE-CREATION src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc src/slave/containerizer/isolators/network/port_mapping.cpp 432b05ce5a99c8239fafc47a6b65d46a0fbac26e src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 Diff: https://reviews.apache.org/r/35482/diff/ Testing --- sudo make check Thanks, Paul Brett
