Re: Review Request 56016: Added a note about task status updates in scheduler and internal API.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 14, 2017, 2:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56016/
> ---
> 
> (Updated March 14, 2017, 2:05 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> d04924a49d0bf28952af6cb72d972cac61e6f781 
>   include/mesos/v1/scheduler/scheduler.proto 
> 6e8246da0af9097b6fd2fe7c9c15fc4bdc9e4fce 
>   src/messages/messages.proto 508ff5960eb70b1299ef5ec02c23852c0088d295 
> 
> 
> Diff: https://reviews.apache.org/r/56016/diff/4/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57594: Added a helper for building a task status from scratch.

[Vinod Kone]

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


Fix it, then Ship it!





src/common/protobuf_utils.hpp
Lines 101-102 (patched)


I would make these the first 2 args because they are the most important and 
cannot be deduced; uuid and tiemstamp could have default values.


- Vinod Kone


On March 14, 2017, 2:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57594/
> ---
> 
> (Updated March 14, 2017, 2:05 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
>   src/docker/executor.cpp 6c11320fc40ba1eebdbdf95f0a52ac383646d9f8 
>   src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
> 
> 
> Diff: https://reviews.apache.org/r/57594/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56208: Updated checks library with general check support.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 14, 2017, 2:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> ---
> 
> (Updated March 14, 2017, 2:05 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
> 
> 
> Diff: https://reviews.apache.org/r/56208/diff/9/
> 
> 
> Testing
> ---
> 
> https://reviews.apache.org/r/56213/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57149: Added a warning if command executor gets an unknown acknowledgement.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 14, 2017, 2:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57149/
> ---
> 
> (Updated March 14, 2017, 2:06 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 
> 
> 
> Diff: https://reviews.apache.org/r/57149/diff/3/
> 
> 
> Testing
> ---
> 
> https://reviews.apache.org/r/56213/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57596: Kept TaskInfo beyond first scheduler ack in command executor.

[Vinod Kone]

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




src/launcher/executor.cpp
Line 383 (original), 383 (patched)


i would rather use `_task` here and below because it gets harder to see 
that `taskData.taskInfo` is `Some` the deeper you go into this function.



src/launcher/executor.cpp
Lines 886 (patched)


Add a comment to signify what this boolean captures.



src/launcher/executor.cpp
Lines 887 (patched)


since `acknowledged` doesn't make sense without `taskInfo`, I would make 
`taskInfo` non-optional and make `taskData` optional.


- Vinod Kone


On March 14, 2017, 2:08 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57596/
> ---
> 
> (Updated March 14, 2017, 2:08 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, command executor wipes TaskInfo after receiving
> a status update acknowledgement from the scheduler to indicate that
> there are no unacknowledged tasks. Keeping original TaskInfo beyond
> the ack can be beneficial, hence we introduce a struct TaskData that
> holds TaskInfo and explicit ack flag.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 
> 
> 
> Diff: https://reviews.apache.org/r/57596/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56213/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56212: Added support for general checks to command executor.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> ---
> 
> (Updated March 14, 2017, 2:09 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 
> 
> 
> Diff: https://reviews.apache.org/r/56212/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56213/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57597: Added comments regarding exit status on Windows vs. Posix.

[Vinod Kone]

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




src/checks/checker.cpp
Lines 436-438 (patched)


Why is this comment here and not in the CheckInfo proto like you did with 
Executor Failure?

Also, the first statement looks very similar to what you have in executor 
failure. Can you explicitly say that we return the result of `WEXITSTATUS` here?


- Vinod Kone


On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57597/
> ---
> 
> (Updated March 14, 2017, 2:09 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> d04924a49d0bf28952af6cb72d972cac61e6f781 
>   include/mesos/v1/scheduler/scheduler.proto 
> 6e8246da0af9097b6fd2fe7c9c15fc4bdc9e4fce 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
> 
> 
> Diff: https://reviews.apache.org/r/57597/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57598: Added comment regarding exit status on Windows vs. Posix in libprocess.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57598/
> ---
> 
> (Updated March 14, 2017, 2:09 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> 7117a2730ae8666d0b55cbe05162a0b4e843193c 
> 
> 
> Diff: https://reviews.apache.org/r/57598/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56213: Added check tests for command executor.

[Vinod Kone]

> On March 14, 2017, 6:24 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/check_tests.cpp
> > Lines 74 (patched)
> > 
> >
> > As the resident PowerShell "expert" (heh!), I tested this (and the 
> > second one) and they do what you're expecting.
> > 
> > Just a couple notes:
> > 
> > You might be wanting to use `Test-Path "somepath"` instead, which 
> > returns a boolean for existence of that path, so you can do `if (Test-Path 
> > "somepath")` instead of the gunk around checking `$?`. Also, semicolons are 
> > unnecssary in PowerShell except to separate multiple commands on one line. 
> > If you want it to be _perfect_ we try to prefer the `Camel-Case` of cmdlet 
> > names, but it's whatever.

Thanks Andrew for the reply.

@Alex Do you want to update accordingly?


- Vinod


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


On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> ---
> 
> (Updated March 14, 2017, 2:09 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
>   src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 
> 
> 
> Diff: https://reviews.apache.org/r/56213/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57254: Updated DRFSorter to support hierarchical roles.

[Jay Guo]

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



Per design doc, we always associate framework to a virtual role. In this 
implementation, however, virtual role is created ONLY when the leaf node is 
turned into internal node. Could you clarify a bit?


src/master/allocator/sorter/drf/sorter.cpp
Lines 101 (patched)


It would be nice to add some comments here to help reader understand that 
we are traversing the tree using each element of `roles`, in order to position 
it in the tree, much like what `mkdir -p /path/to/create/dir` would do.


- Jay Guo


On March 14, 2017, 1:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated March 14, 2017, 1:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit replaces the sorter's flat list of clients with a tree of
> client names; this tree represents the hierarchical relationship between
> sorter clients. Each node in the tree contains an (ordered) list of
> pointers to child nodes. The tree might contain nodes that do not
> correspond directly to sorter clients. For example, adding clients "a/b"
> and "c/d" results in the following tree:
> 
> root
>  -> a
>-> b
>  -> c
>-> d
> 
> The `sort` member function still only returns one result for each
> (active) client in the sorter. This is implemented by ensuring that each
> sorter client is associated with a leaf node in the tree. Note that it
> is possible for a leaf node to be transformed into an internal node by a
> subsequent insertion; to handle this case, we "implicitly" create an
> extra child node, which maintains the invariant that each client has a
> leaf node. For example, if the client "a/b/x" is added to the tree
> above, the result is:
> 
> root
>  -> a
>-> b
>  -> .
>  -> x
>  -> c
>-> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at "a/b", which also includes
> "a/b/x".
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/metrics.cpp 
> 15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/hierarchical_allocator_tests.cpp 
> dce619ec49db480685deb1bf8f7faeebe02e25b5 
>   src/tests/master_allocator_tests.cpp 
> 9f3750215f2b72f6148d0c47cdde6a3f7dfb1b50 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57254/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57254: Updated DRFSorter to support hierarchical roles.

[Jay Guo]

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




src/master/allocator/sorter/drf/sorter.cpp
Line 72 (original), 95-97 (patched)


Maybe a `CHECK(!clients.contains(name))` here?


- Jay Guo


On March 14, 2017, 1:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated March 14, 2017, 1:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit replaces the sorter's flat list of clients with a tree of
> client names; this tree represents the hierarchical relationship between
> sorter clients. Each node in the tree contains an (ordered) list of
> pointers to child nodes. The tree might contain nodes that do not
> correspond directly to sorter clients. For example, adding clients "a/b"
> and "c/d" results in the following tree:
> 
> root
>  -> a
>-> b
>  -> c
>-> d
> 
> The `sort` member function still only returns one result for each
> (active) client in the sorter. This is implemented by ensuring that each
> sorter client is associated with a leaf node in the tree. Note that it
> is possible for a leaf node to be transformed into an internal node by a
> subsequent insertion; to handle this case, we "implicitly" create an
> extra child node, which maintains the invariant that each client has a
> leaf node. For example, if the client "a/b/x" is added to the tree
> above, the result is:
> 
> root
>  -> a
>-> b
>  -> .
>  -> x
>  -> c
>-> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at "a/b", which also includes
> "a/b/x".
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/metrics.cpp 
> 15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/hierarchical_allocator_tests.cpp 
> dce619ec49db480685deb1bf8f7faeebe02e25b5 
>   src/tests/master_allocator_tests.cpp 
> 9f3750215f2b72f6148d0c47cdde6a3f7dfb1b50 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57254/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57425: Added pull method to Containerizer.

[Ilya Pronin]

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

(Updated March 15, 2017, 10:41 a.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

New method is intended to be called by the agent for pulling container
image when asked by an operator.

Currently implemented only for MesosContainerizer.


Diffs (updated)
-

  src/slave/containerizer/containerizer.hpp 
4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41 
  src/slave/containerizer/containerizer.cpp 
9024371b6c4228f0903cfeef3bbec736e1a425f8 
  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
12cf1d8b2872ee3ba25c893d5553eaff048eccac 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
7d6c1b93a2c0e265b9344a0fc27f1cf4ed5325f2 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
478ab09751bbb96c8815c021bc818833b96596b0 
  src/tests/containerizer.hpp 63fe2366d5fe79c71cbe62813a9bb0b830e70057 
  src/tests/containerizer.cpp 548da3a8757c9ad3ab115c6b91f8a1f2f5e37144 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ec9f354c8cb6735f319b7dbea2465a31f3a8d64d 


Diff: https://reviews.apache.org/r/57425/diff/2/

Changes: https://reviews.apache.org/r/57425/diff/1-2/


Testing
---

Added `MesosContainerizerProvisionerTest.PullImage` test verifying that 
`MesosContainerizer` calls the required `Store` method. Ran `make check`.


Thanks,

Ilya Pronin

Re: Review Request 57426: Added PULL_CONTAINER_IMAGE agent API call.

[Ilya Pronin]

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

(Updated March 15, 2017, 10:41 a.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

New call is intended to be used by operators for asking an agent to
pre-fetch a container image for later use.


Diffs (updated)
-

  include/mesos/agent/agent.proto 9bac9541acd24e1123ca5dd5925e2a1381d13b4a 
  include/mesos/v1/agent/agent.proto ea9282cf12fbe1c2ddeaa37223e4811685263734 
  src/slave/http.cpp 1ab6f9475af287a6ac09bc615fa466223a52c97d 
  src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb 
  src/slave/validation.cpp 85c43cacf1361d269c28a7fe8bd1da8615949ec8 
  src/tests/api_tests.cpp 29ae1bcf660fb0e03af1d2192484c9ec739f3ef6 


Diff: https://reviews.apache.org/r/57426/diff/2/

Changes: https://reviews.apache.org/r/57426/diff/1-2/


Testing
---

Added `AgentAPITest.PullContainerImage` test. Ran `make check`. Verified 
manually by sending a call curl.


Thanks,

Ilya Pronin

Re: Review Request 57427: Added authorization for PULL_CONTAINER_IMAGE agent API call.

[Ilya Pronin]

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

(Updated March 15, 2017, 10:42 a.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added authorization for PULL_CONTAINER_IMAGE agent API call.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto e75e1879435f1c2bce47a86e9feebf9d051e969b 
  include/mesos/authorizer/authorizer.proto 
736f76d552956f2351ffd40fc51d088dff83f8c8 
  src/authorizer/local/authorizer.cpp be8037299601427e5d5e79f58f77eea3f89579d0 
  src/slave/http.cpp 1ab6f9475af287a6ac09bc615fa466223a52c97d 
  src/tests/api_tests.cpp 29ae1bcf660fb0e03af1d2192484c9ec739f3ef6 
  src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 


Diff: https://reviews.apache.org/r/57427/diff/2/

Changes: https://reviews.apache.org/r/57427/diff/1-2/


Testing
---

Added `AuthorizationTest.PullContainerImage` and 
`AgentAPITest.PullContainerImageUnauthorized` tests. Ran `make check`.

Verified manually by starting the agent with `--authenticate_http_readwrite` 
and sending a call with curl as a principal that is allowed to 
`pull_container_image`.


Thanks,

Ilya Pronin

Re: Review Request 57622: Introduced a Roles tab in the webui.

[Jay Guo]

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




src/webui/master/static/roles.html
Lines 30-31 (patched)


```
  {{role.resources.mem * (1024 * 1024) | dataSize}}
  {{role.resources.disk * (1024 * 1024) | dataSize}}
```


- Jay Guo


On March 15, 2017, 10:05 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57622/
> ---
> 
> (Updated March 15, 2017, 10:05 a.m.)
> 
> 
> Review request for mesos, Jay Guo, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6995
> https://issues.apache.org/jira/browse/MESOS-6995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, this includes the weight, number of frameworks involved
> with the role, and the resource allocation. Longer term this should
> include the quota information and the revocable resources.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01 
>   src/webui/master/static/index.html 7811ecb2c8c4fc5d14c4d5ca690b611290b07c83 
>   src/webui/master/static/js/app.js 73043a8521d2931b639ece11bf3f2b8bccba83d6 
>   src/webui/master/static/js/controllers.js 
> 4e1d07eb6155301c7d7d2b3e030c38156e8210ad 
>   src/webui/master/static/roles.html PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57622/diff/3/
> 
> 
> Testing
> ---
> 
> manual testing.
> 
> 
> File Attachments
> 
> 
> screenshot
>   
> https://reviews.apache.org/media/uploaded/files/2017/03/14/2670df08-47f2-4cbf-8c61-1f897c19fa1c__Screen_Shot_2017-03-14_at_3.26.06_PM.png
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 57596: Kept TaskInfo beyond first scheduler ack in command executor.

[Alexander Rukletsov]

> On March 15, 2017, 9:51 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp
> > Lines 886 (patched)
> > 
> >
> > Add a comment to signify what this boolean captures.

Hmmm, I thought I could spare a comment since it seemed rather obvious. But 
I'll add one if you think it adds value.


- Alexander


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


On March 14, 2017, 2:08 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57596/
> ---
> 
> (Updated March 14, 2017, 2:08 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, command executor wipes TaskInfo after receiving
> a status update acknowledgement from the scheduler to indicate that
> there are no unacknowledged tasks. Keeping original TaskInfo beyond
> the ack can be beneficial, hence we introduce a struct TaskData that
> holds TaskInfo and explicit ack flag.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 
> 
> 
> Diff: https://reviews.apache.org/r/57596/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56213/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57597: Added comments regarding exit status on Windows vs. Posix.

[Alexander Rukletsov]

> On March 15, 2017, 10:04 a.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Lines 436-438 (patched)
> > 
> >
> > Why is this comment here and not in the CheckInfo proto like you did 
> > with Executor Failure?
> > 
> > Also, the first statement looks very similar to what you have in 
> > executor failure. Can you explicitly say that we return the result of 
> > `WEXITSTATUS` here?

Let's have both! Can be valuable until after MESOS-7242 is resolved.


- Alexander


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


On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57597/
> ---
> 
> (Updated March 14, 2017, 2:09 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> d04924a49d0bf28952af6cb72d972cac61e6f781 
>   include/mesos/v1/scheduler/scheduler.proto 
> 6e8246da0af9097b6fd2fe7c9c15fc4bdc9e4fce 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
> 
> 
> Diff: https://reviews.apache.org/r/57597/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56016: Added a note about task status updates in scheduler and internal API.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:44 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
d04924a49d0bf28952af6cb72d972cac61e6f781 
  include/mesos/v1/scheduler/scheduler.proto 
6e8246da0af9097b6fd2fe7c9c15fc4bdc9e4fce 
  src/messages/messages.proto 508ff5960eb70b1299ef5ec02c23852c0088d295 


Diff: https://reviews.apache.org/r/56016/diff/5/

Changes: https://reviews.apache.org/r/56016/diff/4-5/


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 56017: Added a helper for building a task status from an existing one.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:44 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 


Diff: https://reviews.apache.org/r/56017/diff/5/

Changes: https://reviews.apache.org/r/56017/diff/4-5/


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 57394: Overloaded `<<` for `CheckInfo::Type`.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:44 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/type_utils.hpp c7f86ac3a8c166512410d89678a4dd2622771bf0 
  include/mesos/v1/mesos.hpp 07007429be7fc4bee7238f8dc32197ea9c7a3a7a 
  src/common/type_utils.cpp d86d56d4e1d353d6e82ccff89357bf2abec6eded 
  src/v1/mesos.cpp 5b89603e1b760256c584a188e93be31eeaea7ce2 


Diff: https://reviews.apache.org/r/57394/diff/3/

Changes: https://reviews.apache.org/r/57394/diff/2-3/


Testing
---

https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 57594: Added a helper for building a task status from scratch.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:45 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Vinod's comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/docker/executor.cpp 6c11320fc40ba1eebdbdf95f0a52ac383646d9f8 
  src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 


Diff: https://reviews.apache.org/r/57594/diff/2/

Changes: https://reviews.apache.org/r/57594/diff/1-2/


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:46 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Rebased. NNTR.


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


Repository: mesos


Description
---

Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.


Diffs (updated)
-

  src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
  src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 


Diff: https://reviews.apache.org/r/56208/diff/10/

Changes: https://reviews.apache.org/r/56208/diff/9-10/


Testing
---

https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56210: Reused previous task status for health updates in command executor.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:47 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Rebased. NNTR.


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


Repository: mesos


Description
---

When a new health task status update is generated in the executor, we
have to make sure specific data is duplicated from the previous one to
avoid shadowing of those data during reconciliation.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/56210/diff/8/

Changes: https://reviews.apache.org/r/56210/diff/7-8/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56211: Renamed health checker in command executor for clarity.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:47 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Rebased. NNTR.


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


Repository: mesos


Description
---

Also validate internal invariants when health is updated.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/56211/diff/7/

Changes: https://reviews.apache.org/r/56211/diff/6-7/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 57149: Added a warning if command executor gets an unknown acknowledgement.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:46 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Rebased. NNTR.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/57149/diff/4/

Changes: https://reviews.apache.org/r/57149/diff/3-4/


Testing
---

https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 57596: Kept TaskInfo beyond first scheduler ack in command executor.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:48 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Vinod's comments.


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


Repository: mesos


Description
---

Prior to this patch, command executor wipes TaskInfo after receiving
a status update acknowledgement from the scheduler to indicate that
there are no unacknowledged tasks. Keeping original TaskInfo beyond
the ack can be beneficial, hence we introduce a struct TaskData that
holds TaskInfo and explicit ack flag.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/57596/diff/2/

Changes: https://reviews.apache.org/r/57596/diff/1-2/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 57150: Simplified task id procurement in command executor.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:49 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Minor code restructuring.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/57150/diff/4/

Changes: https://reviews.apache.org/r/57150/diff/3-4/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56212: Added support for general checks to command executor.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:51 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Rebased. NNTR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/56212/diff/7/

Changes: https://reviews.apache.org/r/56212/diff/6-7/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 57597: Added comments regarding exit status on Windows vs. Posix.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:51 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Added a comment for `CheckStatusInfo.exit_code`.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto a432eade1e65a7e1c122b413883607a81393ebac 
  include/mesos/scheduler/scheduler.proto 
d04924a49d0bf28952af6cb72d972cac61e6f781 
  include/mesos/v1/mesos.proto 8a733d9368456dfdf4ca77b80048548bfb3228c1 
  include/mesos/v1/scheduler/scheduler.proto 
6e8246da0af9097b6fd2fe7c9c15fc4bdc9e4fce 
  src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 


Diff: https://reviews.apache.org/r/57597/diff/2/

Changes: https://reviews.apache.org/r/57597/diff/1-2/


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 57598: Added comment regarding exit status on Windows vs. Posix in libprocess.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 12:51 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Rebased. NNTR.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
7117a2730ae8666d0b55cbe05162a0b4e843193c 


Diff: https://reviews.apache.org/r/57598/diff/2/

Changes: https://reviews.apache.org/r/57598/diff/1-2/


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 56016: Added a note about task status updates in scheduler and internal API.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56016/
> ---
> 
> (Updated March 15, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> d04924a49d0bf28952af6cb72d972cac61e6f781 
>   include/mesos/v1/scheduler/scheduler.proto 
> 6e8246da0af9097b6fd2fe7c9c15fc4bdc9e4fce 
>   src/messages/messages.proto 508ff5960eb70b1299ef5ec02c23852c0088d295 
> 
> 
> Diff: https://reviews.apache.org/r/56016/diff/5/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57394: Overloaded `<<` for `CheckInfo::Type`.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57394/
> ---
> 
> (Updated March 15, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7f86ac3a8c166512410d89678a4dd2622771bf0 
>   include/mesos/v1/mesos.hpp 07007429be7fc4bee7238f8dc32197ea9c7a3a7a 
>   src/common/type_utils.cpp d86d56d4e1d353d6e82ccff89357bf2abec6eded 
>   src/v1/mesos.cpp 5b89603e1b760256c584a188e93be31eeaea7ce2 
> 
> 
> Diff: https://reviews.apache.org/r/57394/diff/3/
> 
> 
> Testing
> ---
> 
> https://reviews.apache.org/r/56213/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56017: Added a helper for building a task status from an existing one.

[Gastón Kleiman]

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




src/common/protobuf_utils.hpp
Lines 96 (patched)


The implementation doesn't create a new task sttus message. It updates the 
one passed by the user.

Given this behaviour, I find the method name and the comment misleading.


- Gastón Kleiman


On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> ---
> 
> (Updated March 15, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
> 
> 
> Diff: https://reviews.apache.org/r/56017/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57597: Added comments regarding exit status on Windows vs. Posix.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 15, 2017, 12:51 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57597/
> ---
> 
> (Updated March 15, 2017, 12:51 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a432eade1e65a7e1c122b413883607a81393ebac 
>   include/mesos/scheduler/scheduler.proto 
> d04924a49d0bf28952af6cb72d972cac61e6f781 
>   include/mesos/v1/mesos.proto 8a733d9368456dfdf4ca77b80048548bfb3228c1 
>   include/mesos/v1/scheduler/scheduler.proto 
> 6e8246da0af9097b6fd2fe7c9c15fc4bdc9e4fce 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
> 
> 
> Diff: https://reviews.apache.org/r/57597/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56017: Added a helper for building a task status from an existing one.

[Vinod Kone]

> On March 15, 2017, 1:19 p.m., Gastón Kleiman wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96 (patched)
> > 
> >
> > The implementation doesn't create a new task sttus message. It updates 
> > the one passed by the user.
> > 
> > Given this behaviour, I find the method name and the comment misleading.

How about we take const & TaskStatus as arg. Then the name  makes more sense?


- Vinod


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


On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> ---
> 
> (Updated March 15, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
> 
> 
> Diff: https://reviews.apache.org/r/56017/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56017: Added a helper for building a task status from an existing one.

[Gastón Kleiman]

> On March 15, 2017, 1:19 p.m., Gastón Kleiman wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96 (patched)
> > 
> >
> > The implementation doesn't create a new task sttus message. It updates 
> > the one passed by the user.
> > 
> > Given this behaviour, I find the method name and the comment misleading.
> 
> Vinod Kone wrote:
> How about we take const & TaskStatus as arg. Then the name  makes more 
> sense?

Yes, I like the idea of taking `const TaskStatus&` as arg and creating a copy.


- Gastón


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


On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> ---
> 
> (Updated March 15, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
> 
> 
> Diff: https://reviews.apache.org/r/56017/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57596: Kept TaskInfo beyond first scheduler ack in command executor.

[Vinod Kone]

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


Fix it, then Ship it!





src/launcher/executor.cpp
Line 361 (original), 363 (patched)


s/_task/task/



src/launcher/executor.cpp
Lines 860 (patched)


s/received/received for any status updates/



src/launcher/executor.cpp
Lines 861 (patched)


can you set this to false in the constructor initializer list instead?


- Vinod Kone


On March 15, 2017, 12:48 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57596/
> ---
> 
> (Updated March 15, 2017, 12:48 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, command executor wipes TaskInfo after receiving
> a status update acknowledgement from the scheduler to indicate that
> there are no unacknowledged tasks. Keeping original TaskInfo beyond
> the ack can be beneficial, hence we introduce a struct TaskData that
> holds TaskInfo and explicit ack flag.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 
> 
> 
> Diff: https://reviews.apache.org/r/57596/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56213/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56474: Added the 'CombinedAuthenticator'.

[Vinod Kone]

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




src/authentication/http/combined_authenticator.cpp
Lines 146 (patched)


call this `extractUnauthorizedHeaders` ?



src/authentication/http/combined_authenticator.cpp
Lines 345 (patched)


new line.



include/mesos/authentication/http/combined_authenticator.hpp
Lines 123-130 (patched)


why do we need the `overrides`? do we expect devs to subclass this in the 
future?



src/authentication/http/combined_authenticator.cpp
Lines 237-240 (patched)


You could've done

```
combinedResult.unauthorized = Unauthorized(
extractUnauthorizedHeaders(results),
strings::join("\n\n", extractUnauthorizedBodies(results)));
```

provided `extractUnauthorizedHeaders` returned a vector. can the extract 
functions return a vector instead of list?


- Vinod Kone


On March 14, 2017, 9:36 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> ---
> 
> (Updated March 14, 2017, 9:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION 
>   src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267 
>   src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01 
>   src/authentication/http/combined_authenticator.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56474/diff/8/
> 
> 
> Testing
> ---
> 
> Testing information can be found in the subsequent patch in this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Vinod Kone]

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


Fix it, then Ship it!





src/common/http.cpp
Line 964 (original), 957 (patched)


This function is not "loading" the authenticator so logging so is a bit 
weird. maybe call it "creating" or move this statement to 
`initializeAuthenticators`



src/common/http.cpp
Line 987 (original), 977 (patched)


ditto. not "loading".


- Vinod Kone


On March 14, 2017, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56476/
> ---
> 
> (Updated March 14, 2017, 5:07 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the Mesos code to allow master and agent
> to load multiple HTTP authenticator modules.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf 
> 
> 
> Diff: https://reviews.apache.org/r/56476/diff/6/
> 
> 
> Testing
> ---
> 
> Still need to add tests which test the master and agent with multiple 
> authenticator modules loaded.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56475: Added a test for the 'CombinedAuthenticator'.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/http_authentication_tests.cpp
Lines 284-286 (patched)


can you not use the `Unauthorized` constructor that takes 2 arguments?



src/tests/http_authentication_tests.cpp
Lines 315-316 (patched)


ditto. use 2 arg constructor?



src/tests/http_authentication_tests.cpp
Lines 379 (patched)


// The first authenticator fails but the second one succeeds.



src/tests/http_authentication_tests.cpp
Lines 533 (patched)


nice test!


- Vinod Kone


On March 14, 2017, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56475/
> ---
> 
> (Updated March 14, 2017, 5:07 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a unit test to verify the functionality of
> the `CombinedAuthenticator`. The new test is called
> `CombinedAuthenticatorTest.MultipleAuthenticators`.
> 
> 
> Diffs
> -
> 
>   src/tests/http_authentication_tests.cpp 
> 0eeed9d19881cf3fa5fec7fb7fedc1e92784f58b 
> 
> 
> Diff: https://reviews.apache.org/r/56475/diff/5/
> 
> 
> Testing
> ---
> 
> The following commands were used to test:
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*CombinedAuthenticator*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

reviews@mesos.apache.org

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and Vinod Kone.


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


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/slave/containerizer/composing.hpp 
292374aefbc70e4b9d8c81740656864dbada8e32 
  src/slave/containerizer/composing.cpp 
46539e4671101f9c51de563f52700e974537b472 


Diff: https://reviews.apache.org/r/57644/diff/1/


Testing
---

`make check` in Linux + manual testing


Thanks,

Gastón Kleiman

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

[Gastón Kleiman]

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

(Updated March 15, 2017, 2:58 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description (updated)
---

See the summary.


Diffs (updated)
-

  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 


Diff: https://reviews.apache.org/r/56288/diff/4/

Changes: https://reviews.apache.org/r/56288/diff/3-4/


Testing
---

None, not a functional change.


Thanks,

Gastón Kleiman

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

[Gastón Kleiman]

> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp
> > Lines 617 (patched)
> > 
> >
> > do you want to add a TODO here to not re-use the ContainerID?
> 
> Gastón Kleiman wrote:
> I don't know if we should commit this with that TODO, or if I should wait 
> until MESOS-7120 is resolved, and then update this patch to not re-use the 
> ContainerID.

Added a TODO and addressed it in https://reviews.apache.org/r/57647/.


- Gastón


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


On March 6, 2017, 4:23 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated March 6, 2017, 4:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
>   src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
>   src/tests/health_check_tests.cpp 56e90747f2c943daee675738428d8ddeeafde36d 
> 
> 
> Diff: https://reviews.apache.org/r/55901/diff/13/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 57645: Added support for pausing health checks.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


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


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
  src/docker/executor.cpp 6c11320fc40ba1eebdbdf95f0a52ac383646d9f8 
  src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/57645/diff/1/


Testing
---

`make check` in Linux


Thanks,

Gastón Kleiman

Review Request 57646: Made COMMAND health checks resilient to agent failovers.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


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


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 


Diff: https://reviews.apache.org/r/57646/diff/1/


Testing
---

`make check` in Linux


Thanks,

Gastón Kleiman

Review Request 57647: Made CMD health checks not reuse the check `ContainerID`.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


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


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 


Diff: https://reviews.apache.org/r/57647/diff/1/


Testing
---

`make check` in Linux


Thanks,

Gastón Kleiman

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

[Gastón Kleiman]

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

(Updated March 15, 2017, 3:05 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Rebase + added a TODO.


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


Repository: mesos


Description (updated)
---

See the summary.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
  src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
  src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171 


Diff: https://reviews.apache.org/r/55901/diff/14/

Changes: https://reviews.apache.org/r/55901/diff/13-14/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 57631: Augmented a test to check protobuf::stripAllocationInfo.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57340, 57630, 57631]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 15, 2017, 3:27 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57631/
> ---
> 
> (Updated March 15, 2017, 3:27 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7048
> https://issues.apache.org/jira/browse/MESOS-7048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An unit test is augmented to check both injectAllocationInfo and
> stripAllocationInfo.
> 
> 
> Diffs
> -
> 
>   src/tests/protobuf_utils_tests.cpp 2c32d56e41d12a26824aaf22ef78f8c0048acc00 
> 
> 
> Diff: https://reviews.apache.org/r/57631/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57624: Modified an erroneous comment in the Master API proto.

[Zhitao Li]

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


Ship it!




Ship It!

- Zhitao Li


On March 14, 2017, 10:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57624/
> ---
> 
> (Updated March 14, 2017, 10:27 p.m.)
> 
> 
> Review request for mesos and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `AGENT_REMOVED` event is not sent when the agent does not
> reregister within the timeout. In the future, we would introduce
> a new event that is sent when an agent is marked as unreachable.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 0d8d58983c1ce5553845b0a01df33dca3ac90433 
>   include/mesos/v1/master/master.proto 
> da6f1104fabfe6c09a4cb4ae09ed91bc70827a71 
> 
> 
> Diff: https://reviews.apache.org/r/57624/diff/1/
> 
> 
> Testing
> ---
> 
> Not a functional change.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

reviews@mesos.apache.org

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/containerizer/composing.cpp
Lines 714 (patched)


s/Parent/Root/


- Vinod Kone


On March 15, 2017, 2:58 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57644/
> ---
> 
> (Updated March 15, 2017, 2:58 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7248
> https://issues.apache.org/jira/browse/MESOS-7248
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See the summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> 292374aefbc70e4b9d8c81740656864dbada8e32 
>   src/slave/containerizer/composing.cpp 
> 46539e4671101f9c51de563f52700e974537b472 
> 
> 
> Diff: https://reviews.apache.org/r/57644/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` in Linux + manual testing
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

reviews@mesos.apache.org

[Gastón Kleiman]

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

(Updated March 15, 2017, 4:18 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


Changes
---

Vinod's comment.


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


Repository: mesos


Description (updated)
---

Implemented 'ComposingContainerizer::remove(const ContainerID&)'.


Diffs (updated)
-

  src/slave/containerizer/composing.hpp 
292374aefbc70e4b9d8c81740656864dbada8e32 
  src/slave/containerizer/composing.cpp 
46539e4671101f9c51de563f52700e974537b472 


Diff: https://reviews.apache.org/r/57644/diff/2/

Changes: https://reviews.apache.org/r/57644/diff/1-2/


Testing
---

`make check` in Linux + manual testing


Thanks,

Gastón Kleiman

Review Request 57648: Improved failure message in `MesosContainerizer::remove()`.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and Vinod Kone.


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


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
12cf1d8b2872ee3ba25c893d5553eaff048eccac 


Diff: https://reviews.apache.org/r/57648/diff/1/


Testing
---


Thanks,

Gastón Kleiman

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

[Gastón Kleiman]

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

(Updated March 15, 2017, 4:22 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Improved the wording of what's logged on command health check timeouts.


Diffs (updated)
-

  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 


Diff: https://reviews.apache.org/r/56288/diff/5/

Changes: https://reviews.apache.org/r/56288/diff/4-5/


Testing
---

None, not a functional change.


Thanks,

Gastón Kleiman

Re: Review Request 57648: Improved failure message in `MesosContainerizer::remove()`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 15, 2017, 4:22 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57648/
> ---
> 
> (Updated March 15, 2017, 4:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7248
> https://issues.apache.org/jira/browse/MESOS-7248
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See the summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 12cf1d8b2872ee3ba25c893d5553eaff048eccac 
> 
> 
> Diff: https://reviews.apache.org/r/57648/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57254: Updated DRFSorter to support hierarchical roles.

[Neil Conway]

> On March 15, 2017, 10:16 a.m., Jay Guo wrote:
> > Per design doc, we always associate framework to a virtual role. In this 
> > implementation, however, virtual role is created ONLY when the leaf node is 
> > turned into internal node. Could you clarify a bit?

Nothing has changed conceptually from the design doc. The initial 
implementation always created virtual leaf nodes, but performance benchmarking 
showed that this causes a major regression in sorter performance (~50% on a 
sorter microbenchmark, around 10% slower for the allocator benchmarks). The 
current approach is to create virtual leaf nodes "lazily", only when needed. A 
framework is always associated with a _leaf node_; that leaf node is only a 
_virtual leaf node_ when that is necessary.


- Neil


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


On March 14, 2017, 5:59 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated March 14, 2017, 5:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit replaces the sorter's flat list of clients with a tree of
> client names; this tree represents the hierarchical relationship between
> sorter clients. Each node in the tree contains an (ordered) list of
> pointers to child nodes. The tree might contain nodes that do not
> correspond directly to sorter clients. For example, adding clients "a/b"
> and "c/d" results in the following tree:
> 
> root
>  -> a
>-> b
>  -> c
>-> d
> 
> The `sort` member function still only returns one result for each
> (active) client in the sorter. This is implemented by ensuring that each
> sorter client is associated with a leaf node in the tree. Note that it
> is possible for a leaf node to be transformed into an internal node by a
> subsequent insertion; to handle this case, we "implicitly" create an
> extra child node, which maintains the invariant that each client has a
> leaf node. For example, if the client "a/b/x" is added to the tree
> above, the result is:
> 
> root
>  -> a
>-> b
>  -> .
>  -> x
>  -> c
>-> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at "a/b", which also includes
> "a/b/x".
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/metrics.cpp 
> 15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/hierarchical_allocator_tests.cpp 
> dce619ec49db480685deb1bf8f7faeebe02e25b5 
>   src/tests/master_allocator_tests.cpp 
> 9f3750215f2b72f6148d0c47cdde6a3f7dfb1b50 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57254/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

reviews@mesos.apache.org

[Alexander Rukletsov]

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




src/slave/containerizer/composing.cpp
Lines 499-502 (original), 507-510 (patched)


Could you please make failure messages consistent? Probably in the next RR.


- Alexander Rukletsov


On March 15, 2017, 4:18 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57644/
> ---
> 
> (Updated March 15, 2017, 4:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7248
> https://issues.apache.org/jira/browse/MESOS-7248
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented 'ComposingContainerizer::remove(const ContainerID&)'.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> 292374aefbc70e4b9d8c81740656864dbada8e32 
>   src/slave/containerizer/composing.cpp 
> 46539e4671101f9c51de563f52700e974537b472 
> 
> 
> Diff: https://reviews.apache.org/r/57644/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` in Linux + manual testing
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 57651: Update http authenticator tests to work with any http response.

[Silas Snider]

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

Review request for mesos.


Repository: mesos


Description
---

Update http authenticator tests to work with any http response.


Diffs
-

  src/tests/http_authentication_tests.cpp 
0eeed9d19881cf3fa5fec7fb7fedc1e92784f58b 


Diff: https://reviews.apache.org/r/57651/diff/1/


Testing
---


Thanks,

Silas Snider

Re: Review Request 57651: Update http authenticator tests to work with any http response.

[Silas Snider]

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

(Updated March 15, 2017, 4:47 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Update http authenticator tests to work with any http response.


Diffs
-

  src/tests/http_authentication_tests.cpp 
0eeed9d19881cf3fa5fec7fb7fedc1e92784f58b 


Diff: https://reviews.apache.org/r/57651/diff/1/


Testing
---


Thanks,

Silas Snider

Review Request 57652: Allow authenticators to return any http Response.

[Silas Snider]

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

Review request for mesos.


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


Repository: mesos


Description
---

Previously only allowed Unauthorized/Forbidden.


Diffs
-

  3rdparty/libprocess/include/process/authenticator.hpp 
272d92117547512328c09dfc04c6ca4bf7b6b937 


Diff: https://reviews.apache.org/r/57652/diff/1/


Testing
---


Thanks,

Silas Snider

reviews@mesos.apache.org

[Gastón Kleiman]

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

(Updated March 15, 2017, 4:54 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


Changes
---

Added a comment and made the failure message locally consistent.


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


Repository: mesos


Description
---

Implemented 'ComposingContainerizer::remove(const ContainerID&)'.


Diffs (updated)
-

  src/slave/containerizer/composing.hpp 
292374aefbc70e4b9d8c81740656864dbada8e32 
  src/slave/containerizer/composing.cpp 
46539e4671101f9c51de563f52700e974537b472 


Diff: https://reviews.apache.org/r/57644/diff/3/

Changes: https://reviews.apache.org/r/57644/diff/2-3/


Testing
---

`make check` in Linux + manual testing


Thanks,

Gastón Kleiman

Re: Review Request 57534: Added and implemented RegisterAgent ACL.

[Anindya Sinha]

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


Ship it!




Ship It!

- Anindya Sinha


On March 15, 2017, 12:40 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57534/
> ---
> 
> (Updated March 15, 2017, 12:40 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, and Greg 
> Mann.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added and implemented RegisterAgent ACL.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> e75e1879435f1c2bce47a86e9feebf9d051e969b 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> be8037299601427e5d5e79f58f77eea3f89579d0 
>   src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 
> 
> 
> Diff: https://reviews.apache.org/r/57534/diff/3/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

[Anindya Sinha]

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


Ship it!




Ship It!

- Anindya Sinha


On March 15, 2017, 1:09 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 15, 2017, 1:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/3/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57647: Made CMD health checks not reuse the check `ContainerID`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57644, 56288, 55901, 57645, 57646, 57647]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 15, 2017, 2:59 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57647/
> ---
> 
> (Updated March 15, 2017, 2:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See the summary.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
> 
> 
> Diff: https://reviews.apache.org/r/57647/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` in Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57358: Implemented discard behavior in process::Queue.

[Joseph Wu]

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

(Updated March 15, 2017, 11:01 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Changes
---

Changed approach.  This commit now modifies `process::Queue` and adds logic to 
handle discarded futures in the Queue itself.


Summary (updated)
-

Implemented discard behavior in process::Queue.


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


Repository: mesos


Description (updated)
---

This commit deals with a specific case where the SocketImpl class
passes a self-reference (shared_ptr) into a Future continuation and
then discards the original Future. The behavior of `Future::discard`
is that the `onDiscard` event is only chained to the future immediately
following the discarded future.  i.e.

```
Future top;
top
  .onDiscard([]() { /* This gets executed. */ })

  .then([](const A&) { return Future(); })
  .onDiscard([]() { /* This also gets executed. */ })

  .then([](const B&) { return Future(); })
  .onDiscard([]() { /* But not this. */ });

top.discard();
```

When a Future is discarded, we only clear the `onDiscard` callbacks,
which means that all other continuations will continue to reside in
memory.  In the case of the `Socket` class, the Libevent SSL socket
implementation had several levels of continuations:

```
// Each item in this queue is backed by a Promise<>::Future.
Queue>> accept_queue;

// LibeventSSLSocketImpl::accept.
accept_queue.get()
  .then(...)

// Socket::accept. This continuation holds a self-reference
// to the `shared_ptr`.
  .then([self](...) {...})
```

Because the second continuation is never discarded nor called, we
end up with a dangling pointer.  For the `Socket` class, this leads
to an FD leak.

This commit fixes the self-reference by implementing discard
handlers in the libprocess Queue class.  When a waiting `Queue::get`
is discarded, the Promise backing the `Queue::get` is lazily dropped
and cleaned up.  Data on the queue will then skip these discarded
Promises and satisfy the next pending waiter.


Diffs (updated)
-

  3rdparty/libprocess/include/process/queue.hpp 
ab08e30df742412f22a06202526f8b55350ed435 
  3rdparty/libprocess/src/process.cpp f6ee24e2db43d63d91222549efee85421bbf9bf3 
  3rdparty/libprocess/src/tests/queue_tests.cpp 
95b738133fa50641f8f9b83014837d2808e0e4ff 


Diff: https://reviews.apache.org/r/57358/diff/3/

Changes: https://reviews.apache.org/r/57358/diff/2-3/


Testing
---

cmake .. -DENABLE_LIBEVENT=1 -DENABLE_SSL=1

make libprocess-tests

3rdparty/libprocess/src/tests/libprocess-tests 
--gtest_filter="Scheme/HTTPTest.Endpoints/0" --gtest_repeat="`ulimit -n`"

make check


Thanks,

Joseph Wu

Re: Review Request 57652: Allow authenticators to return any http Response.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

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

- Mesos Reviewbot


On March 15, 2017, 9:47 a.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57652/
> ---
> 
> (Updated March 15, 2017, 9:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-7247
> https://issues.apache.org/jira/browse/MESOS-7247
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously only allowed Unauthorized/Forbidden.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/57652/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 56474: Added the 'CombinedAuthenticator'.

[Greg Mann]

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

(Updated March 15, 2017, 6:17 p.m.)


Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
Vinod Kone.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.


Diffs (updated)
-

  include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION 
  src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267 
  src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01 
  src/authentication/http/combined_authenticator.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56474/diff/9/

Changes: https://reviews.apache.org/r/56474/diff/8-9/


Testing
---

Testing information can be found in the subsequent patch in this chain.


Thanks,

Greg Mann

Re: Review Request 56474: Added the 'CombinedAuthenticator'.

[Greg Mann]

> On March 15, 2017, 1:50 p.m., Vinod Kone wrote:
> > include/mesos/authentication/http/combined_authenticator.hpp
> > Lines 123-130 (patched)
> > 
> >
> > why do we need the `overrides`? do we expect devs to subclass this in 
> > the future?

The `override` specifier is useful here because it tells the compiler that this 
function must override a virtual function of the base class, otherwise the 
compiler will throw an error. It prevents situations in which the signature of 
the base class function is changed but this one is left the same, in which case 
this function would not override anything.


- Greg


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


On March 15, 2017, 6:17 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> ---
> 
> (Updated March 15, 2017, 6:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION 
>   src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267 
>   src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01 
>   src/authentication/http/combined_authenticator.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56474/diff/9/
> 
> 
> Testing
> ---
> 
> Testing information can be found in the subsequent patch in this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Greg Mann]

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

(Updated March 15, 2017, 6:18 p.m.)


Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
Vinod Kone.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This patch updates the Mesos code to allow master and agent
to load multiple HTTP authenticator modules.


Diffs (updated)
-

  src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf 


Diff: https://reviews.apache.org/r/56476/diff/7/

Changes: https://reviews.apache.org/r/56476/diff/6-7/


Testing
---

Still need to add tests which test the master and agent with multiple 
authenticator modules loaded.


Thanks,

Greg Mann

Re: Review Request 56475: Added a test for the 'CombinedAuthenticator'.

[Greg Mann]

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

(Updated March 15, 2017, 6:20 p.m.)


Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
Vinod Kone.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This patch adds a unit test to verify the functionality of
the `CombinedAuthenticator`. The new test is called
`CombinedAuthenticatorTest.MultipleAuthenticators`.


Diffs (updated)
-

  src/tests/http_authentication_tests.cpp 
0eeed9d19881cf3fa5fec7fb7fedc1e92784f58b 


Diff: https://reviews.apache.org/r/56475/diff/6/

Changes: https://reviews.apache.org/r/56475/diff/5-6/


Testing
---

The following commands were used to test:
`make check`
`bin/mesos-tests.sh --gtest_filter="*CombinedAuthenticator*" --gtest_repeat=-1 
--gtest_break_on_failure`


Thanks,

Greg Mann

Re: Review Request 55896: Stop storing agent flags in the XFS disk isolator.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 153 (original), 153 (patched)


s/workDir/_workDir/


- Jiang Yan Xu


On Jan. 24, 2017, 5:10 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55896/
> ---
> 
> (Updated Jan. 24, 2017, 5:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5116
> https://issues.apache.org/jira/browse/MESOS-5116
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We only need the agent work directory so there's no reason
> to store a full copy of the agent flags.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 52f0459421a45b01ce38b17c689633301cd97982 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 
> 
> 
> Diff: https://reviews.apache.org/r/55896/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56474: Added the 'CombinedAuthenticator'.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 15, 2017, 6:17 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> ---
> 
> (Updated March 15, 2017, 6:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION 
>   src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267 
>   src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01 
>   src/authentication/http/combined_authenticator.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56474/diff/9/
> 
> 
> Testing
> ---
> 
> Testing information can be found in the subsequent patch in this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56475: Added a test for the 'CombinedAuthenticator'.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 15, 2017, 6:20 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56475/
> ---
> 
> (Updated March 15, 2017, 6:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a unit test to verify the functionality of
> the `CombinedAuthenticator`. The new test is called
> `CombinedAuthenticatorTest.MultipleAuthenticators`.
> 
> 
> Diffs
> -
> 
>   src/tests/http_authentication_tests.cpp 
> 0eeed9d19881cf3fa5fec7fb7fedc1e92784f58b 
> 
> 
> Diff: https://reviews.apache.org/r/56475/diff/6/
> 
> 
> Testing
> ---
> 
> The following commands were used to test:
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*CombinedAuthenticator*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57629: Fixed a webui bug where the maintenance tab is not shown as active.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 14, 2017, 7:06 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57629/
> ---
> 
> (Updated March 14, 2017, 7:06 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> 4e1d07eb6155301c7d7d2b3e030c38156e8210ad 
> 
> 
> Diff: https://reviews.apache.org/r/57629/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 57623: Updated the maintenance tab in the webui to auto-refresh.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 14, 2017, 3:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57623/
> ---
> 
> (Updated March 14, 2017, 3:30 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follows the established approach used in the other controllers.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> 4e1d07eb6155301c7d7d2b3e030c38156e8210ad 
> 
> 
> Diff: https://reviews.apache.org/r/57623/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 57254: Updated DRFSorter to support hierarchical roles.

[Neil Conway]

> On March 15, 2017, 10:16 a.m., Jay Guo wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 101 (patched)
> > 
> >
> > It would be nice to add some comments here to help reader understand 
> > that we are traversing the tree using each element of `roles`, in order to 
> > position it in the tree, much like what `mkdir -p /path/to/create/dir` 
> > would do.

Thanks, good suggestion.


- Neil


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


On March 14, 2017, 5:59 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated March 14, 2017, 5:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit replaces the sorter's flat list of clients with a tree of
> client names; this tree represents the hierarchical relationship between
> sorter clients. Each node in the tree contains an (ordered) list of
> pointers to child nodes. The tree might contain nodes that do not
> correspond directly to sorter clients. For example, adding clients "a/b"
> and "c/d" results in the following tree:
> 
> root
>  -> a
>-> b
>  -> c
>-> d
> 
> The `sort` member function still only returns one result for each
> (active) client in the sorter. This is implemented by ensuring that each
> sorter client is associated with a leaf node in the tree. Note that it
> is possible for a leaf node to be transformed into an internal node by a
> subsequent insertion; to handle this case, we "implicitly" create an
> extra child node, which maintains the invariant that each client has a
> leaf node. For example, if the client "a/b/x" is added to the tree
> above, the result is:
> 
> root
>  -> a
>-> b
>  -> .
>  -> x
>  -> c
>-> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at "a/b", which also includes
> "a/b/x".
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/metrics.cpp 
> 15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/hierarchical_allocator_tests.cpp 
> dce619ec49db480685deb1bf8f7faeebe02e25b5 
>   src/tests/master_allocator_tests.cpp 
> 9f3750215f2b72f6148d0c47cdde6a3f7dfb1b50 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57254/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57254: Updated DRFSorter to support hierarchical roles.

[Neil Conway]

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

(Updated March 15, 2017, 6:49 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Tweak comments, CHECKs.


Repository: mesos


Description
---

This commit replaces the sorter's flat list of clients with a tree of
client names; this tree represents the hierarchical relationship between
sorter clients. Each node in the tree contains an (ordered) list of
pointers to child nodes. The tree might contain nodes that do not
correspond directly to sorter clients. For example, adding clients "a/b"
and "c/d" results in the following tree:

root
 -> a
   -> b
 -> c
   -> d

The `sort` member function still only returns one result for each
(active) client in the sorter. This is implemented by ensuring that each
sorter client is associated with a leaf node in the tree. Note that it
is possible for a leaf node to be transformed into an internal node by a
subsequent insertion; to handle this case, we "implicitly" create an
extra child node, which maintains the invariant that each client has a
leaf node. For example, if the client "a/b/x" is added to the tree
above, the result is:

root
 -> a
   -> b
 -> .
 -> x
 -> c
   -> d

The "." leaf node holds the allocation that has been made to the "a/b"
client itself; the "a/b" node holds the sum of all the allocations that
have been made to the subtree rooted at "a/b", which also includes
"a/b/x".


Diffs (updated)
-

  src/master/allocator/sorter/drf/metrics.cpp 
15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/hierarchical_allocator_tests.cpp 
dce619ec49db480685deb1bf8f7faeebe02e25b5 
  src/tests/master_allocator_tests.cpp 9f3750215f2b72f6148d0c47cdde6a3f7dfb1b50 
  src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 


Diff: https://reviews.apache.org/r/57254/diff/10/

Changes: https://reviews.apache.org/r/57254/diff/9-10/


Testing
---


Thanks,

Neil Conway

Re: Review Request 53369: Agent cgroup assignment should precede agent initialization.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On March 2, 2017, 10:07 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> ---
> 
> (Updated March 2, 2017, 10:07 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
> 
> 
> Diffs
> -
> 
>   src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d 
>   src/slave/slave.cpp 6ae9458cc81a7623a1837cd636156434a972004b 
> 
> 
> Diff: https://reviews.apache.org/r/53369/diff/5/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57648: Improved failure message in `MesosContainerizer::remove()`.

[Gastón Kleiman]

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

(Updated March 15, 2017, 8:15 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


Repository: mesos


Description
---

See the summary.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
12cf1d8b2872ee3ba25c893d5553eaff048eccac 


Diff: https://reviews.apache.org/r/57648/diff/1/


Testing
---


Thanks,

Gastón Kleiman

reviews@mesos.apache.org

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On March 15, 2017, 4:54 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57644/
> ---
> 
> (Updated March 15, 2017, 4:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7248
> https://issues.apache.org/jira/browse/MESOS-7248
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented 'ComposingContainerizer::remove(const ContainerID&)'.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> 292374aefbc70e4b9d8c81740656864dbada8e32 
>   src/slave/containerizer/composing.cpp 
> 46539e4671101f9c51de563f52700e974537b472 
> 
> 
> Diff: https://reviews.apache.org/r/57644/diff/3/
> 
> 
> Testing
> ---
> 
> `make check` in Linux + manual testing
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57596: Kept TaskInfo beyond first scheduler ack in command executor.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 8:48 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Prior to this patch, command executor wipes TaskInfo after receiving
a status update acknowledgement from the scheduler to indicate that
there are no unacknowledged tasks. Keeping original TaskInfo beyond
the ack can be beneficial, hence we introduce a struct TaskData that
holds TaskInfo and explicit ack flag.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/57596/diff/3/

Changes: https://reviews.apache.org/r/57596/diff/2-3/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56212: Added support for general checks to command executor.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 8:49 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/56212/diff/8/

Changes: https://reviews.apache.org/r/56212/diff/7-8/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 57150: Simplified task id procurement in command executor.

[Alexander Rukletsov]

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

(Updated March 15, 2017, 8:49 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


Diff: https://reviews.apache.org/r/57150/diff/5/

Changes: https://reviews.apache.org/r/57150/diff/4-5/


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 57622: Introduced a Roles tab in the webui.

[Benjamin Mahler]

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

(Updated March 15, 2017, 9:03 p.m.)


Review request for mesos, Jay Guo, Michael Park, and Neil Conway.


Changes
---

Fixed the memory and disk display.


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


Repository: mesos


Description
---

Initially, this includes the weight, number of frameworks involved
with the role, and the resource allocation. Longer term this should
include the quota information and the revocable resources.


Diffs (updated)
-

  src/Makefile.am 8c67e472ec2e973b66c0004f6bf58bd8b0e4fad3 
  src/webui/master/static/index.html 7811ecb2c8c4fc5d14c4d5ca690b611290b07c83 
  src/webui/master/static/js/app.js 73043a8521d2931b639ece11bf3f2b8bccba83d6 
  src/webui/master/static/js/controllers.js 
ae3e7875258fed9e70b8d15ac82b3e8ffadc6522 
  src/webui/master/static/roles.html PRE-CREATION 


Diff: https://reviews.apache.org/r/57622/diff/4/

Changes: https://reviews.apache.org/r/57622/diff/3-4/


Testing
---

manual testing.


File Attachments


screenshot
  
https://reviews.apache.org/media/uploaded/files/2017/03/14/2670df08-47f2-4cbf-8c61-1f897c19fa1c__Screen_Shot_2017-03-14_at_3.26.06_PM.png


Thanks,

Benjamin Mahler

Re: Review Request 57622: Introduced a Roles tab in the webui.

[Benjamin Mahler]

> On March 15, 2017, 10:44 a.m., Jay Guo wrote:
> > src/webui/master/static/roles.html
> > Lines 30-31 (patched)
> > 
> >
> > ```
> >   {{role.resources.mem * (1024 * 1024) | dataSize}}
> >   {{role.resources.disk * (1024 * 1024) | dataSize}}
> > ```

Good catch, thanks!


- Benjamin


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


On March 15, 2017, 9:03 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57622/
> ---
> 
> (Updated March 15, 2017, 9:03 p.m.)
> 
> 
> Review request for mesos, Jay Guo, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6995
> https://issues.apache.org/jira/browse/MESOS-6995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, this includes the weight, number of frameworks involved
> with the role, and the resource allocation. Longer term this should
> include the quota information and the revocable resources.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8c67e472ec2e973b66c0004f6bf58bd8b0e4fad3 
>   src/webui/master/static/index.html 7811ecb2c8c4fc5d14c4d5ca690b611290b07c83 
>   src/webui/master/static/js/app.js 73043a8521d2931b639ece11bf3f2b8bccba83d6 
>   src/webui/master/static/js/controllers.js 
> ae3e7875258fed9e70b8d15ac82b3e8ffadc6522 
>   src/webui/master/static/roles.html PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57622/diff/4/
> 
> 
> Testing
> ---
> 
> manual testing.
> 
> 
> File Attachments
> 
> 
> screenshot
>   
> https://reviews.apache.org/media/uploaded/files/2017/03/14/2670df08-47f2-4cbf-8c61-1f897c19fa1c__Screen_Shot_2017-03-14_at_3.26.06_PM.png
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56017: Added a helper for building a task status from an existing one.

[Alexander Rukletsov]

> On March 15, 2017, 1:19 p.m., Gastón Kleiman wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96 (patched)
> > 
> >
> > The implementation doesn't create a new task sttus message. It updates 
> > the one passed by the user.
> > 
> > Given this behaviour, I find the method name and the comment misleading.
> 
> Vinod Kone wrote:
> How about we take const & TaskStatus as arg. Then the name  makes more 
> sense?
> 
> Gastón Kleiman wrote:
> Yes, I like the idea of taking `const TaskStatus&` as arg and creating a 
> copy.

The implementation does create a new task status message, but from the given 
one: the compiler will make a copy of the provided task status. The task status 
passed by user is left intact. Taking `const TaskStatus&` and creating a copy 
manually is effectively the same, however, less verbose than taking task status 
by value.


- Alexander


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


On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> ---
> 
> (Updated March 15, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
> 
> 
> Diff: https://reviews.apache.org/r/56017/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56681: Use glog to log EXIT() messages.

[Jiang Yan Xu]

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



LGTM.

So in a couple of main.cpp files, this is used prior to glog initialization, 
which would result in this warning "WARNING: Logging before InitGoogleLogging() 
is written to STDERR" but I feel this is an improvement in general.

In "Testing Done", could you attach a sample output that confirms the format 
change?


3rdparty/stout/include/stout/exit.hpp
Line 26 (original), 25 (patched)


s/provides a stream for output/provides a glog stream for output/

So now that's it's using GLOG, the output rules are subject to the GLOG 
configuration. Probably worth a brief mention since this detail is hidden under 
the interface.



3rdparty/stout/include/stout/exit.hpp
Line 36 (original), 35 (patched)


`s/char */char*/`


- Jiang Yan Xu


On Feb. 14, 2017, 1:04 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56681/
> ---
> 
> (Updated Feb. 14, 2017, 1:04 p.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Bugs: MESOS-7115
> https://issues.apache.org/jira/browse/MESOS-7115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use glog to log EXIT() messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/exit.hpp 
> 762864afd8f86f7e1f439ef6ea7e3965a5f147d5 
> 
> 
> Diff: https://reviews.apache.org/r/56681/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 57663: Updated documentation for multiple HTTP authenticators.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Updated documentation for multiple HTTP authenticators.


Diffs
-

  docs/authentication.md 1574db981d5f8ddd7d1f6bef1c2b032823d17297 


Diff: https://reviews.apache.org/r/57663/diff/1/


Testing
---


Thanks,

Greg Mann

Re: Review Request 57358: Implemented discard behavior in process::Queue.

[Benjamin Mahler]

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



Can you split the change to add discard semantics to queue? That patch seems 
orthogonal and doesn't need the big description about the socket problem. I 
think it's a pretty natural change to describe on its own.


3rdparty/libprocess/include/process/queue.hpp
Lines 80-86 (patched)


This looks racy:

(1) `put()` hits line 48, found the promise without a discard request.

(2) Caller discards future corresponding to the promise found in (1), 
executes promise->discard.

(3) `put()` hits line 59 and drops the `t` item on the floor.

Perhaps we could implement the expensive discard approach with a TODO to 
optimize it, if it's simpler? Alternatively, we need to lock this section, but 
we need to make sure we don't deadlock (will let you think more about that).



3rdparty/libprocess/src/process.cpp
Lines 942-947 (patched)


Can you comment on who is doing this discard? Or why it surfaces as 
discarded? The implication in your comment seems to be that this is the case 
where libprocess is finalizing? Couldn't quite follow why that's the case.


- Benjamin Mahler


On March 15, 2017, 6:01 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57358/
> ---
> 
> (Updated March 15, 2017, 6:01 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-6919
> https://issues.apache.org/jira/browse/MESOS-6919
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit deals with a specific case where the SocketImpl class
> passes a self-reference (shared_ptr) into a Future continuation and
> then discards the original Future. The behavior of `Future::discard`
> is that the `onDiscard` event is only chained to the future immediately
> following the discarded future.  i.e.
> 
> ```
> Future top;
> top
>   .onDiscard([]() { /* This gets executed. */ })
> 
>   .then([](const A&) { return Future(); })
>   .onDiscard([]() { /* This also gets executed. */ })
> 
>   .then([](const B&) { return Future(); })
>   .onDiscard([]() { /* But not this. */ });
> 
> top.discard();
> ```
> 
> When a Future is discarded, we only clear the `onDiscard` callbacks,
> which means that all other continuations will continue to reside in
> memory.  In the case of the `Socket` class, the Libevent SSL socket
> implementation had several levels of continuations:
> 
> ```
> // Each item in this queue is backed by a Promise<>::Future.
> Queue>> accept_queue;
> 
> // LibeventSSLSocketImpl::accept.
> accept_queue.get()
>   .then(...)
> 
> // Socket::accept. This continuation holds a self-reference
> // to the `shared_ptr`.
>   .then([self](...) {...})
> ```
> 
> Because the second continuation is never discarded nor called, we
> end up with a dangling pointer.  For the `Socket` class, this leads
> to an FD leak.
> 
> This commit fixes the self-reference by implementing discard
> handlers in the libprocess Queue class.  When a waiting `Queue::get`
> is discarded, the Promise backing the `Queue::get` is lazily dropped
> and cleaned up.  Data on the queue will then skip these discarded
> Promises and satisfy the next pending waiter.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/queue.hpp 
> ab08e30df742412f22a06202526f8b55350ed435 
>   3rdparty/libprocess/src/process.cpp 
> f6ee24e2db43d63d91222549efee85421bbf9bf3 
>   3rdparty/libprocess/src/tests/queue_tests.cpp 
> 95b738133fa50641f8f9b83014837d2808e0e4ff 
> 
> 
> Diff: https://reviews.apache.org/r/57358/diff/3/
> 
> 
> Testing
> ---
> 
> cmake .. -DENABLE_LIBEVENT=1 -DENABLE_SSL=1
> 
> make libprocess-tests
> 
> 3rdparty/libprocess/src/tests/libprocess-tests 
> --gtest_filter="Scheme/HTTPTest.Endpoints/0" --gtest_repeat="`ulimit -n`"
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 57664: Added the '--executor_secret_key' agent flag.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Added a new agent flag, `--executor_secret_key` to allow the
specification of a secret key to be used when generating and
authenticating default executor tokens.


Diffs
-

  docs/configuration.md 9f747406e91840335f97c0166ca75373e47d319b 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp 4637ca62b257921c1e1f7019864d968f07fca8a1 


Diff: https://reviews.apache.org/r/57664/diff/1/


Testing
---

make check


Thanks,

Greg Mann

Review Request 57615: Added support for auth tokens to the V1 executor library.

[Greg Mann]

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

Review request for mesos, Anand Mazumdar, Jan Schlicht, and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds support for authentication tokens to the
V1 default executor library. The tokens are loaded from a
pre-determined environment variable, if present.


Diffs
-

  src/executor/executor.cpp 35378ec4b55ce0427e95f6b868baa6233c9068cd 


Diff: https://reviews.apache.org/r/57615/diff/1/


Testing
---

make check


Thanks,

Greg Mann

Review Request 57665: Renamed a constant.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Renamed the constant containing the default basic HTTP
authenticator's name to accommodate the addition of the
default JWT authenticator.


Diffs
-

  src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3 
  src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf 
  src/master/flags.cpp d25cfdd00346d314c2255035e01b4a61275b0ab2 
  src/slave/flags.cpp 4637ca62b257921c1e1f7019864d968f07fca8a1 


Diff: https://reviews.apache.org/r/57665/diff/1/


Testing
---

make check


Thanks,

Greg Mann

Review Request 57666: Added agent flag to enable executor authentication.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds a new agent flag, `--authenticate_http_executors`,
which requires authentication on the V1 executor API and loads the
default JWT authenticator.


Diffs
-

  docs/configuration.md 9f747406e91840335f97c0166ca75373e47d319b 
  src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3 
  src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp 4637ca62b257921c1e1f7019864d968f07fca8a1 
  src/slave/slave.cpp 425d33e8c7c8c53afe6c60bca083df6d09d8657e 


Diff: https://reviews.apache.org/r/57666/diff/1/


Testing
---

make check


Thanks,

Greg Mann

Review Request 57667: Added executor authentication to the docs.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Added executor authentication to the docs.


Diffs
-

  docs/authentication.md 1574db981d5f8ddd7d1f6bef1c2b032823d17297 


Diff: https://reviews.apache.org/r/57667/diff/1/


Testing
---


Thanks,

Greg Mann

Re: Review Request 57615: Added support for auth tokens to the V1 executor library.

[Greg Mann]

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

(Updated March 15, 2017, 10:09 p.m.)


Review request for mesos, Anand Mazumdar, Jan Schlicht, and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds support for authentication tokens to the
V1 default executor library. The tokens are loaded from a
pre-determined environment variable, if present.


Diffs
-

  src/executor/executor.cpp 35378ec4b55ce0427e95f6b868baa6233c9068cd 


Diff: https://reviews.apache.org/r/57615/diff/1/


Testing (updated)
---

make check

Once MESOS-6999 is resolved, additional tests must be implemented to verify 
that executor authentication is performed correctly.


Thanks,

Greg Mann

Re: Review Request 57427: Added authorization for PULL_CONTAINER_IMAGE agent API call.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57425, 57426, 57427]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 15, 2017, 10:42 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57427/
> ---
> 
> (Updated March 15, 2017, 10:42 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, Jie Yu, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-2824
> https://issues.apache.org/jira/browse/MESOS-2824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for PULL_CONTAINER_IMAGE agent API call.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> e75e1879435f1c2bce47a86e9feebf9d051e969b 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> be8037299601427e5d5e79f58f77eea3f89579d0 
>   src/slave/http.cpp 1ab6f9475af287a6ac09bc615fa466223a52c97d 
>   src/tests/api_tests.cpp 29ae1bcf660fb0e03af1d2192484c9ec739f3ef6 
>   src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 
> 
> 
> Diff: https://reviews.apache.org/r/57427/diff/2/
> 
> 
> Testing
> ---
> 
> Added `AuthorizationTest.PullContainerImage` and 
> `AgentAPITest.PullContainerImageUnauthorized` tests. Ran `make check`.
> 
> Verified manually by starting the agent with `--authenticate_http_readwrite` 
> and sending a call with curl as a principal that is allowed to 
> `pull_container_image`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 57666: Added agent flag to enable executor authentication.

[Greg Mann]

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

(Updated March 15, 2017, 11:25 p.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds a new agent flag, `--authenticate_http_executors`,
which requires authentication on the V1 executor API and loads the
default JWT authenticator.


Diffs (updated)
-

  docs/configuration.md 9f747406e91840335f97c0166ca75373e47d319b 
  src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3 
  src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf 
  src/slave/constants.hpp 1f3c543c3782ee09e13de867f30ffa0a161fb6f0 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp 4637ca62b257921c1e1f7019864d968f07fca8a1 
  src/slave/http.cpp 1ab6f9475af287a6ac09bc615fa466223a52c97d 
  src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb 
  src/slave/slave.cpp 425d33e8c7c8c53afe6c60bca083df6d09d8657e 


Diff: https://reviews.apache.org/r/57666/diff/2/

Changes: https://reviews.apache.org/r/57666/diff/1-2/


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 57666: Added agent flag to enable executor authentication.

[Greg Mann]

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

(Updated March 15, 2017, 11:35 p.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds a new agent flag, `--authenticate_http_executors`,
which requires authentication on the V1 executor API and loads the
default JWT authenticator.


Diffs (updated)
-

  docs/configuration.md 9f747406e91840335f97c0166ca75373e47d319b 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp 4637ca62b257921c1e1f7019864d968f07fca8a1 


Diff: https://reviews.apache.org/r/57666/diff/3/

Changes: https://reviews.apache.org/r/57666/diff/2-3/


Testing
---

make check


Thanks,

Greg Mann

Review Request 57671: Allowed the agent to require executor authentication.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

This patch updates the agent initialization code to make use
of the new `--authenticate_http_executors` flag. When the
flag is set, authentication is required on the executor realm
and the JWT authenticator is loaded.


Diffs
-

  src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3 
  src/common/http.cpp ce32ff36ee58b19f2cb11d80e69ab1ff007e75ef 
  src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b 


Diff: https://reviews.apache.org/r/57671/diff/1/


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 57667: Added executor authentication to the docs.

[Greg Mann]

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

(Updated March 15, 2017, 11:39 p.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Added executor authentication to the docs.


Diffs (updated)
-

  docs/authentication.md 1574db981d5f8ddd7d1f6bef1c2b032823d17297 


Diff: https://reviews.apache.org/r/57667/diff/2/

Changes: https://reviews.apache.org/r/57667/diff/1-2/


Testing
---


Thanks,

Greg Mann

Re: Review Request 57666: Added the '--authenticate_http_executors' agent flag.

[Greg Mann]

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

(Updated March 15, 2017, 11:40 p.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


Summary (updated)
-

Added the '--authenticate_http_executors' agent flag.


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


Repository: mesos


Description
---

This patch adds a new agent flag, `--authenticate_http_executors`,
which requires authentication on the V1 executor API and loads the
default JWT authenticator.


Diffs
-

  docs/configuration.md 9f747406e91840335f97c0166ca75373e47d319b 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp 4637ca62b257921c1e1f7019864d968f07fca8a1 


Diff: https://reviews.apache.org/r/57666/diff/3/


Testing
---

make check


Thanks,

Greg Mann