Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 10, 2017, 6:40 p.m.) Review request for mesos, Alexander

Re: Review Request 56525: Fixed a crash on the agent when handling the SIGUSR1 signal.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56525/#review165161 --- Ship it! I tested as well and the segfault is gone.

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote: > > src/checks/health_checker.cpp, lines 653-654 > > > > > > Why not `defer`? Added `defer` where it corresponds. > On Feb. 9, 2017, 3:20 p.m., Alexander

Re: Review Request 56537: Added a test to ensure framework can upgrade to support MULTI_ROLE.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56537/#review165160 --- Patch looks great! Reviews applied: [56537] Passed command:

Re: Review Request 53993: Updated quota doc to support quota update.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53993/#review165140 --- Patch looks great! Reviews applied: [52284, 53679, 53691, 52103,

Re: Review Request 56527: Disallowed special path components in IDs.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56527/#review165142 --- Ship it! Ship It! - James Peach On Feb. 10, 2017, 7:05

Re: Review Request 56527: Disallowed special path components in IDs.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56527/#review165141 --- src/common/validation.cpp (line 36)

Review Request 56551: Updated CHANGELOG with still experimental features from 1.1.0.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56551/ --- Review request for mesos and Adam B. Repository: mesos Description ---

Re: Review Request 56533: Fix the shell script to not use deprecated [..] in increment operation.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56533/#review165137 --- Ship it! Ship It! - Jiang Yan Xu On Feb. 9, 2017, 11:26

Re: Review Request 56524: Added `drop` for overload to avoid custom logging.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56524/#review165176 --- src/master/master.cpp (lines 2246 - 2247)

Re: Review Request 56362: Windows: Create the Windows container launcher.

> On Feb. 10, 2017, 12:24 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/windows_launcher.cpp, line 148 > > > > > > Ditto about the constructor. Ditto on the fix. - Andrew

Re: Review Request 56362: Windows: Create the Windows container launcher.

> On Feb. 10, 2017, 12:24 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/windows_launcher.cpp, line 78 > > > > > > Since you are assigning the `Container` is this way, you don't actually > > need to

Re: Review Request 56499: Added roles field to framework.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56499/#review165175 --- Ship it! Ship It! - Benjamin Mahler On Feb. 10, 2017, 12:24

Re: Review Request 56499: Added roles field to framework.

> On Feb. 9, 2017, 9:38 p.m., Benjamin Mahler wrote: > > src/master/master.hpp, lines 2425-2451 > > > > > > We need to update the newly introduced `roles` field here. Also, > > oldRoles can be removed in favor of

Re: Review Request 56362: Windows: Create the Windows container launcher.

> On Feb. 10, 2017, 12:24 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/windows_launcher.cpp, lines 59-60 > > > > > > I recall some talk about dis-allowing certain `std::` helpers on the > > `hashmap` and

Re: Review Request 56362: Windows: Create the Windows container launcher.

> On Feb. 10, 2017, 12:24 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/windows_launcher.cpp, lines 59-60 > > > > > > I recall some talk about dis-allowing certain `std::` helpers on the > > `hashmap` and

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56178/#review165182 --- Patch looks great! Reviews applied: [56178] Passed command:

Re: Review Request 56052: Added the 'Secret' protobuf message.

> On Feb. 9, 2017, 6:31 p.m., Jie Yu wrote: > > include/mesos/mesos.proto, line 1994 > > > > > > Actually, one thought here. Given that environment varaible's source > > cannot be a byte stream. Do we want to have

Re: Review Request 56362: Windows: Create the Windows container launcher.

> On Feb. 10, 2017, 12:24 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/windows_launcher.cpp, line 185 > > > > > > s/container.get().handle/container->handle/ Ah thanks, didn't know `Option` had

Re: Review Request 56525: Fixed a crash on the agent when handling the SIGUSR1 signal.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56525/#review165200 --- Ship it! Ship It! - Joseph Wu On Feb. 9, 2017, 9:34 p.m.,

Re: Review Request 55462: Validate resource reservation against allocated role.

> On Feb. 10, 2017, 2:39 a.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 1623-1630 > > > > > > Just to clarify, this is an invariant in that if this fails it is a > > programming error in the

Re: Review Request 56537: Added a test to ensure framework can upgrade to support MULTI_ROLE.

> On Feb. 10, 2017, 9:33 a.m., Jay Guo wrote: > > src/tests/master_tests.cpp, lines 6409-6411 > > > > > > How to check the task is still running here? If no status update is received we know it's still running. To

Re: Review Request 56476: Enabled loading multiple HTTP authenticators in Mesos.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56476/ --- (Updated Feb. 11, 2017, 12:24 a.m.) Review request for mesos, Alexander Rojas,

Re: Review Request 56476: Enabled loading multiple HTTP authenticators in Mesos.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56476/ --- (Updated Feb. 11, 2017, 12:46 a.m.) Review request for mesos, Alexander Rojas,

Re: Review Request 56568: Modified the executor driver to always relink on agent failover.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56568/#review165224 --- Ship it! Ship It! - Vinod Kone On Feb. 11, 2017, 12:45

Re: Review Request 56375: Added a test to ensure offers with different roles cannot be combined.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56375/#review165041 --- src/tests/master_tests.cpp (lines 1905 - 1909)

Review Request 56568: Modified the executor driver to always relink on agent failover.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56568/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-7057

Re: Review Request 56474: Added support for multiple authenticators to libprocess.

> On Feb. 9, 2017, 3:03 p.m., Jan Schlicht wrote: > > 3rdparty/libprocess/src/authenticator_manager.cpp, lines 168-204 > > > > > > How about we make this a function instead of using a lambda? > > Seems to do a

Re: Review Request 56474: Added support for multiple authenticators to libprocess.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56474/ --- (Updated Feb. 11, 2017, 12:44 a.m.) Review request for mesos, Alexander Rojas,

Re: Review Request 56527: Disallowed special path components in IDs.

> On Feb 10, 2017, at 5:02 PM, Jiang Yan Xu wrote: > > > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56527/ > > On February 10th, 2017, 10:11 a.m. PST, James Peach wrote: > > src/common/validation.cpp (Diff revision 1) > namespace

Review Request 56569: Updated `drop` log message from `ERROR` to `WARNING`.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56569/ --- Review request for mesos and Benjamin Mahler. Repository: mesos Description

Re: Review Request 56391: Set AllocationInfo in some reservation-related tests.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56391/#review165202 --- Ship it! Ship It! - Benjamin Mahler On Feb. 10, 2017, 2:57

Re: Review Request 56392: Tightened test expecations in reservation-related tests.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56392/#review165203 --- Patch looks great! Reviews applied: [55461, 56391, 55462, 56392]

Re: Review Request 56392: Tightened test expecations in reservation-related tests.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56392/#review165210 --- Looks good, you'll need to rebase this after you update your

Re: Review Request 56476: Enabled loading multiple HTTP authenticators in Mesos.

> On Feb. 9, 2017, 2:10 p.m., Jan Schlicht wrote: > > src/common/http.cpp, line 910 > > > > > > s/name/httpAuthenticatorName/ > > Just `name` feels too general. I went with the compromise of

Re: Review Request 56568: Modified the executor driver to always relink on agent failover.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56568/#review165219 --- Ship it! +1 for fewer executor deaths during agent recovery

Re: Review Request 56524: Added `drop` for overload to avoid custom logging.

> On 二月 10, 2017, 9:27 p.m., Benjamin Mahler wrote: > > src/master/master.cpp, lines 2246-2247 > > > > > > Can you follow the same format at the drop two lines above? > > > > ``` > > LOG(ERROR) <<

Re: Review Request 56569: Updated `drop` log message from `ERROR` to `WARNING`.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56569/#review165226 --- Ship it! I didn't do an audit but you should probably look to

Re: Review Request 56362: Windows: Create the Windows container launcher.

> On Feb. 9, 2017, 4:24 p.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/windows_launcher.cpp, line 185 > > > > > > s/container.get().handle/container->handle/ > > Andrew Schwartzmeyer wrote: > Ah

Re: Review Request 55710: Added agent capabilities to /state endpoint of master.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55710/#review165217 --- Looks good, the main issue is that it seems error-prone to store

Re: Review Request 56370: Added a test to ensure multi-role framework receiving offers.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56370/#review165229 --- Fix it, then Ship it! Ship It! src/tests/master_tests.cpp

Re: Review Request 55462: Validate resource reservation against allocated role.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55462/#review165204 --- src/master/validation.cpp (lines 1612 - 1629)

Re: Review Request 56378: Added test case for suppress and revive with multi role framework.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56378/#review165214 --- Fix it, then Ship it!

Re: Review Request 56376: Updated allocator test to support create multi role framework.

> On Feb. 8, 2017, 6:48 a.m., Jay Guo wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 203-205 > > > > > > IMO, since this is for testing purpose, we probably don't need to > > rigidly require test

Re: Review Request 56527: Disallowed special path components in IDs.

> On Feb. 10, 2017, 10:11 a.m., James Peach wrote: > > src/common/validation.cpp, line 42 > > > > > > '~' is a bad choice for a ID, but by itself it is not a security issue. > > You ought to check for `id[0] != '~'`

Re: Review Request 56524: Added `drop` for overload to avoid custom logging.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56524/ --- (Updated 二月 11, 2017, 1:02 a.m.) Review request for mesos, Benjamin Mahler and

Re: Review Request 56524: Added `drop` for overload to avoid custom logging.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56524/#review165225 --- Fix it, then Ship it! src/master/master.cpp (line 3278)

Re: Review Request 56052: Added the 'Secret' protobuf message.

> On Feb. 9, 2017, 6:31 p.m., Jie Yu wrote: > > include/mesos/mesos.proto, line 1994 > > > > > > Actually, one thought here. Given that environment varaible's source > > cannot be a byte stream. Do we want to have

Re: Review Request 56569: Made ignoring logging use WARNING in master.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56569/ --- (Updated 二月 11, 2017, 1:52 a.m.) Review request for mesos and Benjamin Mahler.

Re: Review Request 56362: Windows: Create the Windows container launcher.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56362/ --- (Updated Feb. 11, 2017, 2:04 a.m.) Review request for mesos, Alex Clemmer and

Re: Review Request 56551: Updated CHANGELOG with still experimental features from 1.1.0.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56551/#review165228 --- Patch looks great! Reviews applied: [56551] Passed command:

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review165230 --- Patch looks great! Reviews applied: [55900, 56288, 55901]

Re: Review Request 56533: Fix the shell script to not use deprecated [..] in increment operation.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56533/#review165129 --- Patch looks great! Reviews applied: [56533] Passed command:

Re: Review Request 56530: Prevent offers for old agents being sent to MULTI_ROLE frameworks.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56530/#review165070 --- Patch looks great! Reviews applied: [56530] Passed command:

Re: Review Request 56537: Added a test to ensure framework can upgrade to support MULTI_ROLE.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56537/#review165087 --- src/tests/master_tests.cpp (lines 6409 - 6411)

Review Request 56537: Added a test to ensure framework can upgrade to support MULTI_ROLE.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56537/ --- Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 10, 2017, 11:25 a.m.) Review request for mesos, Alexander

Re: Review Request 56288: Improved the wording of what's logged on command health check timeouts.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56288/ --- (Updated Feb. 10, 2017, 11:17 a.m.) Review request for mesos, Alexander

Re: Review Request 55461: Made resource reservation validation multi-role aware.

> On Feb. 6, 2017, 10:28 p.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 1500-1503 > > > > > > Hm.. do you know which call sites need to be updated? It seems like we > > need to do the sweep now

Re: Review Request 56527: Disallowed special path components in IDs.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56527/#review165099 --- Patch looks great! Reviews applied: [56526, 56527] Passed

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

> On Feb. 9, 2017, 1:59 p.m., Benjamin Mahler wrote: > > src/authorizer/local/authorizer.cpp, line 248 > > > > > > Not yours, but I find it rather confusing as to what the object value > > is, looking at the other

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56178/ --- (Updated Feb. 10, 2017, 12:27 p.m.) Review request for mesos, Adam B,

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review164951 --- src/checks/health_checker.cpp (line 657)

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

> On Feb. 9, 2017, 10:59 p.m., Benjamin Mahler wrote: > > src/master/master.cpp, lines 2533-2535 > > > > > > Longer term, are there any thoughts on how we might be able to know > > which role is not authorized?

Re: Review Request 56391: Set AllocationInfo in some reservation-related tests.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56391/ --- (Updated Feb. 10, 2017, 3:57 p.m.) Review request for mesos, Benjamin Mahler,

Re: Review Request 54639: Implemented the 'create()' method of OCI store.

> On Feb. 9, 2017, 7:36 a.m., Jie Yu wrote: > > src/slave/containerizer/mesos/provisioner/oci/paths.hpp, lines 36-39 > > > > > > Can you elaborate a bit more the layout here? > > > > so `layer_id` is the

Re: Review Request 56526: Added stout constants for special path components.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56526/#review165143 --- Ship it! Ship It! - James Peach On Feb. 10, 2017, 7:03

Re: Review Request 56392: Tightened test expecations in reservation-related tests.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56392/ --- (Updated Feb. 10, 2017, 4:16 p.m.) Review request for mesos, Benjamin Mahler,

Re: Review Request 55462: Validate resource reservation against allocated role.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55462/ --- (Updated Feb. 10, 2017, 4:16 p.m.) Review request for mesos, Benjamin Mahler,

Re: Review Request 55462: Validate resource reservation against allocated role.

> On Feb. 10, 2017, 3:39 a.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 1632-1634 > > > > > > Shouldn't we also be checking that the allocation role matches the > > reservation role? Yes, this