Review Request 65280: Fixed make clean without Python.

[Till Toenshoff]

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

Review request for mesos, Benjamin Bannier, Benno Evers, Kapil Arya, and Joseph 
Wu.


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


Repository: mesos


Description
---

see summary.


Diffs
-

  src/Makefile.am fe8f689c6b82a88e6e114d5d2082183b3cf4ed54 


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


Testing
---

Functional testing without Python build artifacts.


Thanks,

Till Toenshoff

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

[Benjamin Mahler]

> On Jan. 23, 2018, 1:29 a.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 10170 (patched)
> > 
> >
> > s/Indicated/Indicates/ ?

Oh, yeah this isn't my comment but I'll touch it up here since it's been moved.


- Benjamin


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


On Jan. 23, 2018, 12:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 23, 2018, 12:11 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8389
> https://issues.apache.org/jira/browse/MESOS-8389
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the past, the notion of a "removable" task meant: the task is
> terminal and acknowledged. However, the `isRemovable` helper only
> defines removability by the task state (terminal or unreachable)
> but not whether the terminal update is acknowledged.
> 
> As a result, the code that is calling `isRemovable` is unintuitive.
> One example of a confusing piece of code is within updateTask. Here,
> we have logic which says, if the task is removable, recover the
> resources but don't remove it. This seems more intuitive if directly
> described as: "if the task is no longer consuming resources (e.g.
> transitioned to terminal or unreachable) then recover the resources".
> 
> If one looks up the documentation of `isRemovable`, it says "When a
> task becomes removable, it is erased from the master's primary task
> data structures", but that isn't accurate since this function doesn't
> say whether the terminal task has been acknowledged, which is
> required for a task to be removable.
> 
> To make an immediate improvement, this patch removes `isRemovable`
> (no pun intended) in favor of directly checking the states we care
> about. In the future, we may want to introduce some helpers like
> `isAllocatedResources` to describe whether the task's resources are
> considered allocated.
> 
> If we do introduce a notion of `isRemovable` in the future, it seems
> it should take the `Task*` so that it can say whether the task could
> be "removed" from the master, which includes checking that terminal
> tasks have been acknowledged. However, "remove" is somewhat of a
> confusing term, since what we're actually doing is "completing" the
> task.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65106/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65111: Added tests to verify the executor is shutdown if no task is delivered.

[Benjamin Mahler]

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



Seems like we need http vs driver based tests, and also need to test the 
failover case?


src/tests/slave_tests.cpp
Lines 4422-4425 (patched)


Any reason to test command vs custom executor? Seems like they both behave 
the same here? Seems we care more about HTTP vs driver-based?



src/tests/slave_tests.cpp
Lines 4527-4529 (patched)


Seems we also need the failover case tested?


- Benjamin Mahler


On Jan. 12, 2018, 2:34 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65111/
> ---
> 
> (Updated Jan. 12, 2018, 2:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the executor is shutdown if all of its
> initial tasks could not be delivered, even after the executor has been
> registered. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 16c75bb0ca570dfb7089743344cf38acdb517705 
>   src/tests/mesos.cpp d751b2e9c635eb6a5039678de426467176cda908 
>   src/tests/slave_tests.cpp 59e3065ad9aadbd90cdfd32e830b433b88d6de86 
> 
> 
> Diff: https://reviews.apache.org/r/65111/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65277: Logged pid of launched containers in the linux launcher.

[Benjamin Mahler]

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



Why does this need to be in a hook gated by the parent<->child synchronization? 
Are you trying to improve the likelihood that it gets logged? It seems simpler 
to just log it after the fork?

- Benjamin Mahler


On Jan. 23, 2018, 1:58 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65277/
> ---
> 
> (Updated Jan. 23, 2018, 1:58 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Logged pid of launched containers in the linux launcher.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e319894874e628beda6dc305462af0c274fd7b 
> 
> 
> Diff: https://reviews.apache.org/r/65277/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 65278: Fixed a race in the test `ROOT_MultiTaskgroupSharePidNamespace`.

[Qian Zhang]

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

In the test `DefaultExecutorTest.ROOT_MultiTaskgroupSharePidNamespace`,
we read the file `ns` in each of the two task's sandbox and check if
their contents (the pid namespace of the task itself) are same. However
it is possible we do the read for the second task after that file is
created but before it is written, i.e., the content we read from the
`ns` file of the second task would be empty which will cause the check
failed.

In this patch, we read the file `ns` for each task in a while loop, and
only break from the loop when we find their contents are same.


Diffs
-

  src/tests/default_executor_tests.cpp 065eae665db08b39aef70325cebc2753beb30d96 


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


Testing
---

Manually ran this test repeatedly (100 times).


Thanks,

Qian Zhang

Review Request 65277: Logged pid of launched containers in the linux launcher.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Logged pid of launched containers in the linux launcher.


Diffs
-

  src/slave/containerizer/mesos/linux_launcher.cpp 
c2e319894874e628beda6dc305462af0c274fd7b 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/master.cpp
Lines 10170 (patched)


s/Indicated/Indicates/ ?



src/master/master.cpp
Lines 10301 (patched)


s/simply/simplify/


- Vinod Kone


On Jan. 23, 2018, 12:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 23, 2018, 12:11 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8389
> https://issues.apache.org/jira/browse/MESOS-8389
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the past, the notion of a "removable" task meant: the task is
> terminal and acknowledged. However, the `isRemovable` helper only
> defines removability by the task state (terminal or unreachable)
> but not whether the terminal update is acknowledged.
> 
> As a result, the code that is calling `isRemovable` is unintuitive.
> One example of a confusing piece of code is within updateTask. Here,
> we have logic which says, if the task is removable, recover the
> resources but don't remove it. This seems more intuitive if directly
> described as: "if the task is no longer consuming resources (e.g.
> transitioned to terminal or unreachable) then recover the resources".
> 
> If one looks up the documentation of `isRemovable`, it says "When a
> task becomes removable, it is erased from the master's primary task
> data structures", but that isn't accurate since this function doesn't
> say whether the terminal task has been acknowledged, which is
> required for a task to be removable.
> 
> To make an immediate improvement, this patch removes `isRemovable`
> (no pun intended) in favor of directly checking the states we care
> about. In the future, we may want to introduce some helpers like
> `isAllocatedResources` to describe whether the task's resources are
> considered allocated.
> 
> If we do introduce a notion of `isRemovable` in the future, it seems
> it should take the `Task*` so that it can say whether the task could
> be "removed" from the master, which includes checking that terminal
> tasks have been acknowledged. However, "remove" is somewhat of a
> confusing term, since what we're actually doing is "completing" the
> task.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65106/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65246: Added download button for master and agent logs in Web UI.

[Benjamin Mahler]

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



Nice to see both, will wait for update from vinod's review.

- Benjamin Mahler


On Jan. 20, 2018, 4:16 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> ---
> 
> (Updated Jan. 20, 2018, 4:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
> https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 
> 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/1/
> 
> 
> Testing
> ---
> 
> Created an High Availability Mode Mesos cluster locally:
> 
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' 
> --work_dir='/tmp/master1' --quorum=1 
> --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' 
> --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' 
> --work_dir='/tmp/master2' --quorum=1 
> --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' 
> --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' 
> --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI 
> endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI:
> ![New logs button](https://i.imgur.com/vo9mXT6.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65203: Updated the CHANGELOG for 1.5.0 release.

[Gilbert Song]

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

(Updated Jan. 22, 2018, 4:23 p.m.)


Review request for mesos, Adam B, Anand Mazumdar, Andrew Schwartzmeyer, 
Benjamin Mahler, Chun-Hung Hsiao, Gaston Kleiman, Greg Mann, Ilya Pronin, Jie 
Yu, James Peach, Kapil Arya, Joseph Wu, Kevin Klues, Neil Conway, Qian Zhang, 
Till Toenshoff, Vinod Kone, Jiang Yan Xu, and Zhitao Li.


Repository: mesos


Description
---

Updated the CHANGELOG for 1.5.0 release.


Diffs (updated)
-

  CHANGELOG 078c5d64100d81ce84dbdff4055cdda465e19011 


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

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


Testing
---

N/A.


Thanks,

Gilbert Song

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

[Benjamin Mahler]

> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> >

Thank you for the review! :)


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.hpp
> > Lines 2208 (patched)
> > 
> >
> > can you use `CHECK_NE` here so that `task->state()` is auto printed 
> > when this fails?

Ditto cases below.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.hpp
> > Lines 2334 (patched)
> > 
> >
> > ditto. `CHECK_NE`?

See my comment below, I avoided it because the CHECK_NE will print the state 
but we don't really need that here. Also added logging of the task/framework id.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 10111-10127 (original), 10111-10119 (patched)
> > 
> >
> > move this to #1061
> > 
> > Also, consider moving this to `protobuf_utils.hpp`. AFAICT this is used 
> > in 4 different places in this diff.

This needs to be up here since we mutate the task state.

I looked into it originally, but when I was writing its documentation it seemed 
strange to need a helper for an or condition and it seemed like it only had 
some meaning according to the master's current semantics around resource 
recovery. So I held off.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 10247-10261 (original), 10245-10261 (patched)
> > 
> >
> > Not yours, but I wish we can remove this if altogether and CHECK that 
> > we are only removing a terminal or unreachable task period. AFAICT, the 
> > only place where we violate this invariant is in `Master::finalize()`. 
> > Maybe add a TODO to fix?

Yeah I was looking to track this down and CHECK here instead, and ended up 
writing a TODO that captures why I couldn't, somehow I had removed that prior 
to publishing the patch. Added one.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Line 11423 (original), 11423 (patched)
> > 
> >
> > CHECK_NE

I avoided it here, since there wasn't a need to print the task state, if this 
fails it's TASK_UNREACHABLE. It seemed a little more readable to see the != 
operator.

Added the task id and framework id here for debuggability.


- Benjamin


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


On Jan. 23, 2018, 12:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 23, 2018, 12:11 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8389
> https://issues.apache.org/jira/browse/MESOS-8389
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the past, the notion of a "removable" task meant: the task is
> terminal and acknowledged. However, the `isRemovable` helper only
> defines removability by the task state (terminal or unreachable)
> but not whether the terminal update is acknowledged.
> 
> As a result, the code that is calling `isRemovable` is unintuitive.
> One example of a confusing piece of code is within updateTask. Here,
> we have logic which says, if the task is removable, recover the
> resources but don't remove it. This seems more intuitive if directly
> described as: "if the task is no longer consuming resources (e.g.
> transitioned to terminal or unreachable) then recover the resources".
> 
> If one looks up the documentation of `isRemovable`, it says "When a
> task becomes removable, it is erased from the master's primary task
> data structures", but that isn't accurate since this function doesn't
> say whether the terminal task has been acknowledged, which is
> required for a task to be removable.
> 
> To make an immediate improvement, this patch removes `isRemovable`
> (no pun intended) in favor of directly checking the states we care
> about. In the future, we may want to introduce some helpers like
> `isAllocatedResources` to describe whether the task's resources are
> considered allocated.
> 
> If we do introduce a notion of `isRemovable` in the future, it seems
> it should take the `Task*` so that it can say whether the task could
> be "removed" from the master, which includes checking that terminal
> tasks have been acknowledged. However, "remove" is somewhat of a
> confusing term, since what we're actually doing is 

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

[Benjamin Mahler]

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

(Updated Jan. 23, 2018, 12:11 a.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Updated per vinod's review.


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


Repository: mesos


Description
---

In the past, the notion of a "removable" task meant: the task is
terminal and acknowledged. However, the `isRemovable` helper only
defines removability by the task state (terminal or unreachable)
but not whether the terminal update is acknowledged.

As a result, the code that is calling `isRemovable` is unintuitive.
One example of a confusing piece of code is within updateTask. Here,
we have logic which says, if the task is removable, recover the
resources but don't remove it. This seems more intuitive if directly
described as: "if the task is no longer consuming resources (e.g.
transitioned to terminal or unreachable) then recover the resources".

If one looks up the documentation of `isRemovable`, it says "When a
task becomes removable, it is erased from the master's primary task
data structures", but that isn't accurate since this function doesn't
say whether the terminal task has been acknowledged, which is
required for a task to be removable.

To make an immediate improvement, this patch removes `isRemovable`
(no pun intended) in favor of directly checking the states we care
about. In the future, we may want to introduce some helpers like
`isAllocatedResources` to describe whether the task's resources are
considered allocated.

If we do introduce a notion of `isRemovable` in the future, it seems
it should take the `Task*` so that it can say whether the task could
be "removed" from the master, which includes checking that terminal
tasks have been acknowledged. However, "remove" is somewhat of a
confusing term, since what we're actually doing is "completing" the
task.


Diffs (updated)
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 65263: Updated `SlaveRecoveryTest.RecoverCompletedExecutor` to verify gc.

[Qian Zhang]

> On Jan. 23, 2018, 4:24 a.m., Vinod Kone wrote:
> > Ship It!
> 
> Vinod Kone wrote:
> To confirm, would this new test have failed without the fix for 
> https://issues.apache.org/jira/browse/MESOS-8460 ?

Actually no. The root cause of MESOS-8460 is we used a framework pointer after 
that framework object is deleted. But when we remove a framework in agent code, 
we actually put the framework pointer into `BoundedHashMap completedFrameworks;`, so the framework object will 
be deleted when the capacity of `completedFrameworks` is reached. The capacity 
of `completedFrameworks` is 50 (`MAX_COMPLETED_FRAMEWORKS`, there is no agent 
flag to modify it), so launching one framework in the test will not cause that 
segfault issue.


- Qian


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


On Jan. 22, 2018, 9:21 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65263/
> ---
> 
> (Updated Jan. 22, 2018, 9:21 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8462
> https://issues.apache.org/jira/browse/MESOS-8462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the test `SlaveRecoveryTest.RecoverCompletedExecutor`, when the
> completed executor is recovered, verify its work and meta directories
> gc'ed successfully.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 6d8a57138903637d3cf9e5b19a466961d2c120d7 
> 
> 
> Diff: https://reviews.apache.org/r/65263/diff/1/
> 
> 
> Testing
> ---
> 
> Manually ran this test repeatedly (100 times).
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65253: Fixed dropped events on the master operator API stream.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 22, 2018, 11:36 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65253/
> ---
> 
> (Updated Jan. 22, 2018, 11:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-8469
> https://issues.apache.org/jira/browse/MESOS-8469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master's `Subscribers::send()` call path previously accessed
> event-related master state after an asynchronous call. Thus, if
> that state changed after the call to `Subscribers::send()` but
> before the event was actually sent, messages could be dropped.
> 
> This issue was observed when a TASK_KILLED update failed to be
> sent on an operator's stream because the task was removed from
> master state in between the aforementioned async calls.
> 
> This patch updates that call path to capture a shared copy of
> event-related metadata so that the asynchronous calls have a
> consistent view of the master's state at the time of the event.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65253/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65255: Updated inverse-offers-framework with authentication.

[Till Toenshoff]

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

(Updated Jan. 22, 2018, 11:42 p.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Vinod Kone.


Changes
---

Removed dependency to prevent complex dependency graph temporarily.


Bugs: MESOS-5362, MESOS-5827 and MESOS-8357
https://issues.apache.org/jira/browse/MESOS-5362
https://issues.apache.org/jira/browse/MESOS-5827
https://issues.apache.org/jira/browse/MESOS-8357


Repository: mesos


Description
---

See summary.


Diffs
-

  src/examples/inverse_offer_framework.cpp PRE-CREATION 


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


Testing
---

make check && tested with external tooling.


Thanks,

Till Toenshoff

Re: Review Request 65253: Fixed dropped events on the master operator API stream.

[Greg Mann]

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

(Updated Jan. 22, 2018, 11:36 p.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.


Changes
---

Addressed BenM's comments.


Summary (updated)
-

Fixed dropped events on the master operator API stream.


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


Repository: mesos


Description (updated)
---

The master's `Subscribers::send()` call path previously accessed
event-related master state after an asynchronous call. Thus, if
that state changed after the call to `Subscribers::send()` but
before the event was actually sent, messages could be dropped.

This issue was observed when a TASK_KILLED update failed to be
sent on an operator's stream because the task was removed from
master state in between the aforementioned async calls.

This patch updates that call path to capture a shared copy of
event-related metadata so that the asynchronous calls have a
consistent view of the master's state at the time of the event.


Diffs (updated)
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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

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


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 65252: Updated the v1/mesos.proto to keep consistancy with general mesos.proto.

[Gilbert Song]

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

(Updated Jan. 22, 2018, 3:21 p.m.)


Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Gaston Kleiman, 
Greg Mann, Jie Yu, Michael Park, and Neil Conway.


Repository: mesos


Description
---

Updated the v1/mesos.proto to keep consistancy with general mesos.proto.


Diffs (updated)
-

  include/mesos/v1/mesos.proto b8e016e60322879e53ffa3bef23481015b0a6d2d 


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

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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 65252: Updated the v1/mesos.proto to keep consistancy with general mesos.proto.

[Gaston Kleiman]

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


Fix it, then Ship it!





include/mesos/v1/mesos.proto
Line 2459 (original), 2470 (patched)


Thanks for the cleanup!

I did a diff, and in the v0 proto there's only one space after the period 
in this line.

So I'd say we should: `s/meta-data.  /meta-data. /`


- Gaston Kleiman


On Jan. 22, 2018, 1:59 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65252/
> ---
> 
> (Updated Jan. 22, 2018, 1:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the v1/mesos.proto to keep consistancy with general mesos.proto.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto b8e016e60322879e53ffa3bef23481015b0a6d2d 
> 
> 
> Diff: https://reviews.apache.org/r/65252/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 65274: Fixed --ip6 and --ip6_discovery_command document location.

[Andrew Schwartzmeyer]

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


Ship it!




But fix the description since it's a copy of the summary right now.

- Andrew Schwartzmeyer


On Jan. 22, 2018, 2:11 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65274/
> ---
> 
> (Updated Jan. 22, 2018, 2:11 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Avinash sridharan.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed --ip6 and --ip6_discovery_command document location.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md b7a242c46159c2b8fe2da04ca25a1942e8b5e961 
>   docs/configuration/master-and-agent.md 
> 53bcead6dcf66eaa04c744817bab9f24418b3d72 
> 
> 
> Diff: https://reviews.apache.org/r/65274/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 64387: Windows: Ported docker health check tests.

[Joseph Wu]

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




src/tests/environment.cpp
Lines 344-348 (patched)


It's a little redundant to pass in an `errorString`.

Also, this method could return a `Future` directly.  
`runNetNamespaceCheck` would do the waiting/error checking.



src/tests/environment.cpp
Lines 378-389 (patched)


So these tests will be disabled until the user runs `docker pull 
microsoft/powershell:nanoserver` prior to running the test suite?



src/tests/environment.cpp
Lines 391-416 (patched)


If you change `launchAndWaitContainer` to return a `Future` type... you can 
do something like this instead:
```
Future> checks = 
  process::collect(
  launchAndWaitContainer(docker, container1, ...),
  launchAndWaitContainer(docker, container2, ...));
  
checks.await(Seconds(30));
// See if both containers succeeded or not...

process::collect(
docker->rm(container1, true)
docker->rm(container2, true))
  .await(Seconds(15)); // Possibly check if this succeeds too.
```

Using `process::collect` would parallelize some of this code.



src/tests/health_check_tests.cpp
Lines 2278-2284 (patched)


Wow... we might not want to enable this test by default if the image is 
that large.


- Joseph Wu


On Jan. 17, 2018, 4:12 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64387/
> ---
> 
> (Updated Jan. 17, 2018, 4:12 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `HealthCheckTest.ROOT_DOCKER_*` and
> `DockerContainerizerHealthCheckTest.*` tests now work on Windows.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp 72bd621f02f97ea5fd553f3dc0bd52adb8ddee8f 
>   src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 
>   src/tests/mesos.hpp 16c75bb0ca570dfb7089743344cf38acdb517705 
> 
> 
> Diff: https://reviews.apache.org/r/64387/diff/8/
> 
> 
> Testing
> ---
> 
> Windows Server:
> [==] Running 5 tests from 2 test cases.
> [--] Global test environment set-up.
> [--] 2 tests from HealthCheckTest
> [ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask
> [   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask (21263 ms)
> [ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> [   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange (23512 ms)
> [--] 2 tests from HealthCheckTest (44835 ms total)
> 
> [--] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest
> [ RUN  ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
> [   OK ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
>  (28487 ms)
> [ RUN  ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0
> [   OK ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTPS/0
>  (26447 ms)
> [ RUN  ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0
> [   OK ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaTCP/0
>  (26264 ms)
> [--] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest 
> (81268 ms total)
> 
> [--] Global test environment tear-down
> [==] 5 tests from 2 test cases ran. (126559 ms total)
> [  PASSED  ] 5 tests
> 
> Rest of tests pass.
> 
> Windows Client (Disabled network health checks):
> Proof that network health checks are skipped on Windows Client.
> C:\Program Files\Docker\Docker\Resources\bin\docker.exe: Error response from 
> daemon: sharing of hyperv containers network is not supported.
> 356b087e7fa640f83fe27ebeb3396bfc7b2bbebd917aeaec0508b887b41d31f4
> -
> We cannot run any Docker health checks tests because:
> Running in another container's namespace is not supported on this version of 
> Windows.
> 
> Rest rests pass.
> 
> Linux:
> make check passes
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65253: Avoided dropping events on the master operator API stream.

[Benjamin Mahler]

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


Fix it, then Ship it!




Nice to see performance improvements here as well :)

In the summary, could you say something like "Fixed ..." to clarify that it's a 
bug fix? Also, maybe we could include in the description the task added/updated 
scenarios?


src/master/master.hpp
Lines 1997-2001 (original), 1997-2004 (patched)


Per offline discussion, maybe a TODO here for overloads?



src/master/master.cpp
Lines 11196-11199 (patched)


Maybe we should put the breaks here to prevent an accident later if someone 
adds the AGENT_ADDED case?



src/master/master.cpp
Lines 11200-11201 (patched)


No default due to MESOS-2664.



src/master/master.cpp
Lines 11200-11201 (patched)


No defaults for enums, due to MESOS-2664.

Ditto below.



src/master/master.cpp
Lines 11200-11201 (patched)


No defaults for enums, due to MESOS-2664.

Ditto below.



src/master/master.cpp
Lines 11205 (patched)


std::move(event)?



src/master/master.cpp
Lines 11346-11348 (original), 11379-11381 (patched)


No default due to MESOS-2664.


- Benjamin Mahler


On Jan. 22, 2018, 9:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65253/
> ---
> 
> (Updated Jan. 22, 2018, 9:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-8469
> https://issues.apache.org/jira/browse/MESOS-8469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master's `Subscribers::send()` call path previously accessed
> event-related master state after an asynchronous call. Thus, if
> that state changed after the call to `Subscribers::send()` but
> before the event was actually sent, messages could be dropped.
> 
> This patch updates that call path to capture a shared copy of
> event-related metadata so that the asynchronous calls have a
> consistent view of the master's state at the time of the event.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65253/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 65274: Fixed --ip6 and --ip6_discovery_command document location.

[Gilbert Song]

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

Review request for mesos, Andrew Schwartzmeyer and Avinash sridharan.


Repository: mesos


Description
---

Fixed --ip6 and --ip6_discovery_command document location.


Diffs
-

  docs/configuration/agent.md b7a242c46159c2b8fe2da04ca25a1942e8b5e961 
  docs/configuration/master-and-agent.md 
53bcead6dcf66eaa04c744817bab9f24418b3d72 


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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 65252: Updated the v1/mesos.proto to keep consistancy with general mesos.proto.

[Gilbert Song]

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

(Updated Jan. 22, 2018, 1:59 p.m.)


Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Gaston Kleiman, 
Greg Mann, Jie Yu, Michael Park, and Neil Conway.


Repository: mesos


Description
---

Updated the v1/mesos.proto to keep consistancy with general mesos.proto.


Diffs (updated)
-

  include/mesos/v1/mesos.proto b8e016e60322879e53ffa3bef23481015b0a6d2d 


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

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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 65273: Added the agent flag --disk_profile_adaptor to agent.md file.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 22, 2018, 9:55 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65273/
> ---
> 
> (Updated Jan. 22, 2018, 9:55 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the agent flag --disk_profile_adaptor to agent.md file.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md b7a242c46159c2b8fe2da04ca25a1942e8b5e961 
> 
> 
> Diff: https://reviews.apache.org/r/65273/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Joseph Wu]

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



This patch will likely need a few tweaks based on how the previous patch is 
changed, so I don't have much to say here.


src/checks/checker_process.cpp
Lines 479-481 (original), 479-487 (patched)


This is a bit unexpected.  Considering that these arguments will be joined 
a few lines down, how is the quoting maintained?


- Joseph Wu


On Jan. 16, 2018, 4:09 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 16, 2018, 4:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Review Request 65273: Added the agent flag --disk_profile_adaptor to agent.md file.

[Gilbert Song]

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

Review request for mesos, Jie Yu and Joseph Wu.


Repository: mesos


Description
---

Added the agent flag --disk_profile_adaptor to agent.md file.


Diffs
-

  docs/configuration/agent.md b7a242c46159c2b8fe2da04ca25a1942e8b5e961 


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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 65197: Added some missing email addresses to the contributors list.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Jan. 22, 2018, 8:45 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65197/
> ---
> 
> (Updated Jan. 22, 2018, 8:45 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, 
> Benjamin Hindman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some missing email addresses to the contributors list.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 8fc6d2bafaa8bef9ea6957e6d352de371e358f1d 
> 
> 
> Diff: https://reviews.apache.org/r/65197/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65202: Adopted the libprocess `DEFAULT_TEST_TIMEOUT`.

[Benjamin Bannier]

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




src/logging/flags.hpp
Lines 22 (patched)


Is this needed?



src/tests/flags.cpp
Lines 37 (patched)


Moving this out of the header is great. Let's do it in a separate, 
preceeding commit.



src/tests/flags.cpp
Lines 161 (patched)


Let's explicitly call out await in the flag name, e.g., something along the 
lines of `default_await_timeout`.



src/tests/flags.cpp
Lines 163 (patched)


We could explicitly call out await here as well, e.g.,

The default timeout for awaiting test events.



src/tests/mesos.hpp
Lines 94-99 (original), 94-100 (patched)


Since this is a header `using` decls lock fundamentally very, very wrong to 
me. Let's not add to this.



src/tests/mesos.hpp
Lines 100 (patched)


Let's not add more `using` decls to this header.


- Benjamin Bannier


On Jan. 18, 2018, 12:55 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65202/
> ---
> 
> (Updated Jan. 18, 2018, 12:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Bugs: MESOS-7016
> https://issues.apache.org/jira/browse/MESOS-7016
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than hard-coding a 15sec test timeout, use the libprocess
> `DEFAULT_TEST_TIMEOUT` variable so that this can be globally
> tuned. Added a test flag `--default_timeout` that can be used
> to set the `DEFAULT_TEST_TIMEOUT` on a test run.
> 
> 
> Diffs
> -
> 
>   src/logging/flags.hpp fe40a11c6a5e3ef44ecf008d1aa03d4e74e59859 
>   src/tests/agent_container_api_tests.cpp 
> 618569277545205017320aaf1f3a70e540d35e30 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 5412a7eccaa836a7f182772f1eb33b10298c96a1 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> d761ea46fd4f12c6bed74090bf5ead7587f16811 
>   src/tests/disk_quota_tests.cpp 4e25e32bf784ea818e18497fe3ebc9f94c5861ff 
>   src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255 
>   src/tests/flags.hpp df4d4588f405ea884bb89021264e70218e054fae 
>   src/tests/flags.cpp 4542f3443830474edd97e8b695af0c61685e947c 
>   src/tests/gc_tests.cpp 0e34c017f2c629b527915218f517b88d8392aa4b 
>   src/tests/main.cpp f945ac9e8858998174b4c2785f6a5612bae66a41 
>   src/tests/master_contender_detector_tests.cpp 
> 015d310b72a152829641d436f7cb558f72dfd77c 
>   src/tests/mesos.hpp 16c75bb0ca570dfb7089743344cf38acdb517705 
>   src/tests/registrar_tests.cpp cbb58c1cd72dc13ed16b5465452d17526255c763 
>   src/tests/slave_recovery_tests.cpp 641fc6e82be19bdea55d35ec8f34a862de58489e 
>   src/tests/slave_tests.cpp 59e3065ad9aadbd90cdfd32e830b433b88d6de86 
> 
> 
> Diff: https://reviews.apache.org/r/65202/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 65253: Avoided dropping events on the master operator API stream.

[Greg Mann]

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

(Updated Jan. 22, 2018, 9:10 p.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

The master's `Subscribers::send()` call path previously accessed
event-related master state after an asynchronous call. Thus, if
that state changed after the call to `Subscribers::send()` but
before the event was actually sent, messages could be dropped.

This patch updates that call path to capture a shared copy of
event-related metadata so that the asynchronous calls have a
consistent view of the master's state at the time of the event.


Diffs
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 65201: Added a global `DEFAULT_TEST_TIMEOUT` variable.

[Benjamin Bannier]

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




3rdparty/libprocess/Makefile.am
Lines 200 (patched)


I don't really like adding this testing tool to `libprocess`. Could you 
introduce some convenience library here the mesos and libprocess tests could 
link to (similar for cmake).



3rdparty/libprocess/include/process/gtest.hpp
Line 249 (original), 251 (patched)


We don't really fully qualify the names here, but instead use just 
`process::...` elsewhere. Let's stick to that, and potentially clean up 
globally if we really want it.

Here and below.



3rdparty/libprocess/include/process/gtest.hpp
Line 446 (original), 448 (patched)


Let's just wrap this macro definition so we do not need to worry about 
accidentally commenting out code when expanding, e.g.,

#define AWAIT_ASSERT_RESPONSE_HEADER_EQ_FOR(expected, key, actual, 
duration) \
  ASSERT_PRED_FORMAT4(  
 \
  AwaitAssertResponseHeaderEq,  
 \
  expected, 
 \
  key,  
 \
  actual,   
 \
  duration)



3rdparty/libprocess/include/process/gtest.hpp
Line 454 (original), 456 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 489 (original), 491 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 497 (original), 499 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 538 (original), 540 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 546 (original), 548 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 620 (original), 622 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 628 (original), 630 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 661 (original), 663 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 702 (original), 704 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 751 (original), 753 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 784 (original), 786 (patched)


Ditto.



3rdparty/libprocess/include/process/gtest.hpp
Line 792 (original), 794 (patched)


Ditto.



3rdparty/libprocess/src/gtest.cpp
Lines 1 (patched)


Let's also add this file to the cmake build setup.


- Benjamin Bannier


On Jan. 18, 2018, 12:54 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65201/
> ---
> 
> (Updated Jan. 18, 2018, 12:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Bugs: MESOS-7016
> https://issues.apache.org/jira/browse/MESOS-7016
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a global `DEFAULT_TEST_TIMEOUT` variable that applications
> can use to globally tune the default timeout in the `AWAIT_READY`
> family of test macros. The default remains 15sec.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am ba9e78148932304ce1961b88e5f97180abd35586 
>   3rdparty/libprocess/include/process/gtest.hpp 
> eee726653d52af2e4a148819e420ebd22e5123a9 
>   3rdparty/libprocess/src/gtest.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65201/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 65197: Added some missing email addresses to the contributors list.

[Gaston Kleiman]

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

(Updated Jan. 22, 2018, 12:45 p.m.)


Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, 
Benjamin Hindman, Greg Mann, and Joseph Wu.


Changes
---

Removed temporary/invalid email addresses.


Repository: mesos


Description
---

Added some missing email addresses to the contributors list.


Diffs (updated)
-

  docs/contributors.yaml 8fc6d2bafaa8bef9ea6957e6d352de371e358f1d 


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

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


Testing
---


Thanks,

Gaston Kleiman

Re: Review Request 65253: Avoided dropping events on the master operator API stream.

[Greg Mann]

> On Jan. 22, 2018, 7:26 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Line 11260 (original), 11292 (patched)
> > 
> >
> > So looks like we do make a copy of the event for 
> > `FRAMEWORK_{ADDED,UPDATED_REMOVED}` and `AGENT_ADDED` after all. By taking 
> > Event as Shared we are only avoiding copies for `TASK_{ADDED,UPDATED}` and 
> > `AGENT_REMOVED`. 
> > 
> > A bit inconsistent, but I guess worth it for the perf improvement?

Indeed, we do copy the event for those event types in order to provide 
authorization-based filtering. However, we could optimize this code by building 
a new event from scratch rather than copying the original and clearing fields. 
I can follow up with a patch for this optimization.

Using `Shared` allows us to eliminate additional copies from the lambda capture 
in `Subscribers::send()`.


- Greg


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


On Jan. 22, 2018, 8:39 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65253/
> ---
> 
> (Updated Jan. 22, 2018, 8:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-8469
> https://issues.apache.org/jira/browse/MESOS-8469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided dropping events on the master operator API stream.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65253/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65253: Avoided dropping events on the master operator API stream.

[Greg Mann]

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

(Updated Jan. 22, 2018, 8:39 p.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Avoided dropping events on the master operator API stream.


Diffs (updated)
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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

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


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 64386: Refactored health checks to take in executor information.

[Joseph Wu]

> On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote:
> > src/checks/checker.hpp
> > Lines 69-70 (original), 73-74 (patched)
> > 
> >
> > These guys can now be part of `MesosRuntimeInfo` : )

And fix the spelling in the comment tweak :)
s/containierizer/containerizer/


> On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.hpp
> > Lines 40-47 (patched)
> > 
> >
> > What meaning do you put in the difference between `UCR` and `MESOS`? 
> > For me, the difference is whether the checker library talks to the task 
> > directly or via a container runtime; as a consequence, different types of 
> > command checks is used.

The `UCR` and `MESOS` enums should be combined for now, as there is no logical 
difference between them at the moment.  (You could actually use nested 
container health checks on "`MESOS`" executors, if you had the necessary 
credentials.)


> On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.hpp
> > Lines 40-69 (patched)
> > 
> >
> > These types remind me of protobufs. I do not envision a need to make 
> > them protobufs in the nearest future. Here is what comes to my mind instead:
> > ```
> > class RuntimeOptions
> > {
> > public:
> >   enum class Type
> >   {
> >   MESOS,
> >   DOCKER,
> >   UCR
> >   };
> >   
> > private:
> >   const Type type;
> > };
> > 
> > class MesosRuntimeOptions: public RuntimeOptions
> > {
> > public:
> >   MesosRuntimeOptions(...): RuntimeOptions(MESOS), ...;
> > 
> > private:
> >   const Option taskPid;
> >   const std::vector namespaces;
> > };
> > 
> > class DockerRuntimeOptions: public RuntimeOptions
> > {
> > public:
> >   DockerRuntimeOptions(...): RuntimeOptions(Docker), ...;
> >   
> > private:
> >   const std::string dockerPath;
> >   const std::string socketName;
> >   const std::string containerName;
> > }
> > ```
> > What do you think? I would also suggest to declare these types in a 
> > separate file to minimize dependencies and hence compilation time. Also, 
> > this should likely be a separate patch ; )

I broadly agree with this refactor...

Subclassing the `RuntimeOptions` means that you'll either need:
(A) The `CheckerProcess::commandCheck()` method to up-cast to 
`DockerRuntimeOptions`.  If we put other information inside these classes (like 
`taskPid` for the `MesosRuntimeOptions`), then there will be lots of up-casting 
all over the Health checker code.

(B) Or we will need to add a virtual method to `RuntimeOptions` to hold the 
logic inside `wrapDockerExec`.  And other virtual methods as necessary to 
indirectly expose the other private fields.

I'm inclined towards (B).


Note: `taskPid` and `namespaces` are used by the Docker executor's health 
checks too.  The `docker exec` is run inside the same `net` namespace as the 
executor's PID.  So there aren't many options specific to the 
`MesosRuntimeOptions` at the moment.


- Joseph


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


On Jan. 17, 2018, 4:10 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64386/
> ---
> 
> (Updated Jan. 17, 2018, 4:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added information about the calling executor to the health check
> library. After the refactoring, the command health check code originally
> in the docker executor is now in the health check library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 

Re: Review Request 65263: Updated `SlaveRecoveryTest.RecoverCompletedExecutor` to verify gc.

[Vinod Kone]

> On Jan. 22, 2018, 8:24 p.m., Vinod Kone wrote:
> > Ship It!

To confirm, would this new test have failed without the fix for 
https://issues.apache.org/jira/browse/MESOS-8460 ?


- Vinod


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


On Jan. 22, 2018, 1:21 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65263/
> ---
> 
> (Updated Jan. 22, 2018, 1:21 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8462
> https://issues.apache.org/jira/browse/MESOS-8462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the test `SlaveRecoveryTest.RecoverCompletedExecutor`, when the
> completed executor is recovered, verify its work and meta directories
> gc'ed successfully.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 6d8a57138903637d3cf9e5b19a466961d2c120d7 
> 
> 
> Diff: https://reviews.apache.org/r/65263/diff/1/
> 
> 
> Testing
> ---
> 
> Manually ran this test repeatedly (100 times).
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65263: Updated `SlaveRecoveryTest.RecoverCompletedExecutor` to verify gc.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 22, 2018, 1:21 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65263/
> ---
> 
> (Updated Jan. 22, 2018, 1:21 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8462
> https://issues.apache.org/jira/browse/MESOS-8462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the test `SlaveRecoveryTest.RecoverCompletedExecutor`, when the
> completed executor is recovered, verify its work and meta directories
> gc'ed successfully.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 6d8a57138903637d3cf9e5b19a466961d2c120d7 
> 
> 
> Diff: https://reviews.apache.org/r/65263/diff/1/
> 
> 
> Testing
> ---
> 
> Manually ran this test repeatedly (100 times).
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65108: Added documentation for `protobuf::isTerminalState`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 12, 2018, 1:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65108/
> ---
> 
> (Updated Jan. 12, 2018, 1:34 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp fd728b7038ffcbb430cd3d635046788ee388e4ec 
> 
> 
> Diff: https://reviews.apache.org/r/65108/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65107: Updated the task state metrics to be more readable.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 12, 2018, 1:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65107/
> ---
> 
> (Updated Jan. 12, 2018, 1:34 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 
> 
> 
> Diff: https://reviews.apache.org/r/65107/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

[Vinod Kone]

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




src/master/master.hpp
Lines 2208 (patched)


can you use `CHECK_NE` here so that `task->state()` is auto printed when 
this fails?



src/master/master.hpp
Lines 2215 (patched)


s/the UI/in the UI/



src/master/master.hpp
Lines 2334 (patched)


ditto. `CHECK_NE`?



src/master/master.cpp
Lines 10111-10127 (original), 10111-10119 (patched)


move this to #1061

Also, consider moving this to `protobuf_utils.hpp`. AFAICT this is used in 
4 different places in this diff.



src/master/master.cpp
Lines 10247-10261 (original), 10245-10261 (patched)


Not yours, but I wish we can remove this if altogether and CHECK that we 
are only removing a terminal or unreachable task period. AFAICT, the only place 
where we violate this invariant is in `Master::finalize()`. Maybe add a TODO to 
fix?



src/master/master.cpp
Line 11423 (original), 11423 (patched)


CHECK_NE



src/master/master.cpp
Lines 11447 (patched)


Can you include taskid and frameworkid in the error message here for easy 
debuggability?


- Vinod Kone


On Jan. 12, 2018, 1:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 12, 2018, 1:34 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8389
> https://issues.apache.org/jira/browse/MESOS-8389
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the past, the notion of a "removable" task meant: the task is
> terminal and acknowledged. However, the `isRemovable` helper only
> defines removability by the task state (terminal or unreachable)
> but not whether the terminal update is acknowledged.
> 
> As a result, the code that is calling `isRemovable` is unintuitive.
> One example of a confusing piece of code is within updateTask. Here,
> we have logic which says, if the task is removable, recover the
> resources but don't remove it. This seems more intuitive if directly
> described as: "if the task is no longer consuming resources (e.g.
> transitioned to terminal or unreachable) then recover the resources".
> 
> If one looks up the documentation of `isRemovable`, it says "When a
> task becomes removable, it is erased from the master's primary task
> data structures", but that isn't accurate since this function doesn't
> say whether the terminal task has been acknowledged, which is
> required for a task to be removable.
> 
> To make an immediate improvement, this patch removes `isRemovable`
> (no pun intended) in favor of directly checking the states we care
> about. In the future, we may want to introduce some helpers like
> `isAllocatedResources` to describe whether the task's resources are
> considered allocated.
> 
> If we do introduce a notion of `isRemovable` in the future, it seems
> it should take the `Task*` so that it can say whether the task could
> be "removed" from the master, which includes checking that terminal
> tasks have been acknowledged. However, "remove" is somewhat of a
> confusing term, since what we're actually doing is "completing" the
> task.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3d5180b8028b2a15ad111f91863e77747b362593 
>   src/master/master.cpp 465336d33008a848df063d4295416eb91f7bb44f 
> 
> 
> Diff: https://reviews.apache.org/r/65106/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Gaston Kleiman]

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




src/tests/api_tests.cpp
Lines 1011-1013 (patched)


This doesn't seem to be necessary in most other tests, why exaclty do we 
need it here?



src/tests/api_tests.cpp
Lines 1116-1119 (patched)


This test doesn't use more than one offer, so we don't need this filter.



src/tests/api_tests.cpp
Lines 6276-6278 (patched)


Ditto.



src/tests/api_tests.cpp
Lines 6398 (patched)


Wouldn't this expectation fail if the master received a terminal operation 
status update before doing the `GET_OPERATIONS` call?

We probably want to drop all `UpdateOperationStatusMessage` protobufs from 
the agent to the salve.


- Gaston Kleiman


On Jan. 22, 2018, 6:02 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65044/
> ---
> 
> (Updated Jan. 22, 2018, 6:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_OPERATIONS' call lists all operations known to master or agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
>   include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
>   include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
>   include/mesos/v1/master/master.proto 
> 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
>   src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
>   src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
>   src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
>   src/tests/api_tests.cpp bdacc30be4dc8656a41a0c47d0e350d48e59ad94 
> 
> 
> Diff: https://reviews.apache.org/r/65044/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65253: Avoided dropping events on the master operator API stream.

[Greg Mann]

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

(Updated Jan. 22, 2018, 7:51 p.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Avoided dropping events on the master operator API stream.


Diffs (updated)
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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

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


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 65245: Renamed `LOG` by `Stream logs` in Web UI.

[Vinod Kone]

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




src/webui/master/static/agent.html
Line 45 (original), 45 (patched)


Hmm. This is inconsistent with how we did the stream vs download for 
sandboxes. The stream is just a hyper link with the filename followed by a 
Download button.

Can we do the same with these logs for consistency? A hyperlink "LOG" that 
opens up pailer (which is what it is today) and a "Download" button next to it 
with some space (like we did for sandboxes).


- Vinod Kone


On Jan. 20, 2018, 4:14 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65245/
> ---
> 
> (Updated Jan. 20, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
> https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The home and agents Web UI are now more descriptive. The name of the
> functions called when clicking these two links have also been updated.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 
> 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65245/diff/1/
> 
> 
> Testing
> ---
> 
> Checked manually that the feature was still working on the home and agents 
> endpoints.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 65252: Updated the v1/mesos.proto to keep consistancy with general mesos.proto.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





include/mesos/v1/mesos.proto
Line 1310 (original), 1308 (patched)


New line here as in `include/mesos/mesos.proto`?


- Chun-Hung Hsiao


On Jan. 20, 2018, 2:05 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65252/
> ---
> 
> (Updated Jan. 20, 2018, 2:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the v1/mesos.proto to keep consistancy with general mesos.proto.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto b8e016e60322879e53ffa3bef23481015b0a6d2d 
> 
> 
> Diff: https://reviews.apache.org/r/65252/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 65253: Avoided dropping events on the master operator API stream.

[Vinod Kone]

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



Please add a test in the next review.


src/master/master.hpp
Line 1997 (original), 1997 (patched)


put the arg on the next line and align with others.

also `const &`?



src/master/master.hpp
Lines 2002-2003 (patched)


`const &` ?



src/master/master.cpp
Line 11159 (original), 11160 (patched)


can you make this an rvalue ref as suggested by @bmahler offline?



src/master/master.cpp
Line 11205 (original), 11248 (patched)


put the arg on next line.



src/master/master.cpp
Line 11260 (original), 11292 (patched)


So looks like we do make a copy of the event for 
`FRAMEWORK_{ADDED,UPDATED_REMOVED}` and `AGENT_ADDED` after all. By taking 
Event as Shared we are only avoiding copies for `TASK_{ADDED,UPDATED}` and 
`AGENT_REMOVED`. 

A bit inconsistent, but I guess worth it for the perf improvement?


- Vinod Kone


On Jan. 22, 2018, 7:14 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65253/
> ---
> 
> (Updated Jan. 22, 2018, 7:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-8469
> https://issues.apache.org/jira/browse/MESOS-8469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master's Subscribers::send() call path previously accessed
> event-related master state after an asynchronous call. Thus, if
> that state changed after the call to Subscribers::send() but
> before the event was actually sent, messages could be dropped.
> 
> 
> This patch updates that call path to capture a shared copy of
> event-related metadata so that the asynchronous calls have a
> consistent view of the master's state at the time of the event.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65253/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 63862: Windows: Ported docker_tests.cpp.

[Joseph Wu]

> On Nov. 30, 2017, 6:33 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Line 318 (original), 391-428 (patched)
> > 
> >
> > LGTM though another maintainer may want to take a look.

The current iteration of this this test (revision 5) looks fine.  (Marking as 
Fixed)


- Joseph


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


On Jan. 5, 2018, 10:33 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> ---
> 
> (Updated Jan. 5, 2018, 10:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ported the disabled tests to run on Windows. The following tests
> could not be ported due to Windows platform limitations and remain
> diabled:
>   - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
> sandbox).
>   - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
>   - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_tests.cpp 
> 0ac4a79e03d5e11ead5a57a9749e26c20a1ddd57 
> 
> 
> Diff: https://reviews.apache.org/r/63862/diff/5/
> 
> 
> Testing
> ---
> 
> Windows mesos-test:
> [==] 754 tests from 77 test cases ran. (270586 ms total)
> [  PASSED  ] 754 tests.
> 
> 
> Windows DockerTest only:
> [==] 11 tests from 1 test case ran. (85617 ms total)
> [  PASSED  ] 11 tests.
> 
> Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia 
> GPU):
> [==] 12 tests from 1 test case ran. (12413 ms total)
> [  PASSED  ] 12 tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63862: Windows: Ported docker_tests.cpp.

[Joseph Wu]

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




src/tests/containerizer/docker_tests.cpp
Lines 61-62 (patched)


Is it possible to run earlier/later versions of images?

i.e. If the base OS is 1710 (or later) can it still run this image?  Or if 
the base OS is 1708 (or earlier) will the image definitely fail?



src/tests/containerizer/docker_tests.cpp
Lines 127 (patched)


Avoid using `CHECK` which can prematurely stop the test suite.  Try 
`ASSERT_TRUE(future.ready());`.



src/tests/containerizer/docker_tests.cpp
Lines 155-163 (patched)


It doesn't really make a difference to the Linux tests whether they mount a 
file or directory.  We might as well make them all mount a directory; and 
thereby remove this helper.



src/tests/containerizer/docker_tests.cpp
Lines 580-582 (original), 712-715 (patched)


Do you know if this will _never_ be supported?
In which case, we should get rid of the `_TEMP_DISABLED_ON_WINDOWS` and 
make the disabling more permanent.


- Joseph Wu


On Jan. 5, 2018, 10:33 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> ---
> 
> (Updated Jan. 5, 2018, 10:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ported the disabled tests to run on Windows. The following tests
> could not be ported due to Windows platform limitations and remain
> diabled:
>   - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
> sandbox).
>   - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
>   - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_tests.cpp 
> 0ac4a79e03d5e11ead5a57a9749e26c20a1ddd57 
> 
> 
> Diff: https://reviews.apache.org/r/63862/diff/5/
> 
> 
> Testing
> ---
> 
> Windows mesos-test:
> [==] 754 tests from 77 test cases ran. (270586 ms total)
> [  PASSED  ] 754 tests.
> 
> 
> Windows DockerTest only:
> [==] 11 tests from 1 test case ran. (85617 ms total)
> [  PASSED  ] 11 tests.
> 
> Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia 
> GPU):
> [==] 12 tests from 1 test case ran. (12413 ms total)
> [  PASSED  ] 12 tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65253: Avoided dropping events on the master operator API stream.

[Greg Mann]

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

(Updated Jan. 22, 2018, 7:14 p.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

The master's Subscribers::send() call path previously accessed
event-related master state after an asynchronous call. Thus, if
that state changed after the call to Subscribers::send() but
before the event was actually sent, messages could be dropped.


This patch updates that call path to capture a shared copy of
event-related metadata so that the asynchronous calls have a
consistent view of the master's state at the time of the event.


Diffs (updated)
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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

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


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 65045: Tested correct operation handling during master failover.

[Benjamin Bannier]

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



This test emitted some gmock warnings for me. Could you get rid of these?

GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: registered(0x7fff12024248, @0x7fe5e0006de0 
622fd6fc-f57a-4657-8d79-d30efb8f8e66-, @0x7fe5e0006940 id: 
"622fd6fc-f57a-4657-8d79-d30efb8f8e66"
Ip: 3492307904
Port: 37105
Pid: "master@192.99.40.208:37105"
Hostname: "gru1.hw.ca1.mesosphere.com"
Version: "1.6.0"
Address {
  hostname: "gru1.hw.ca1.mesosphere.com"
  ip: "192.99.40.208"
  port: 37105
}
Capabilities {
  type: AGENT_UPDATE
}
)
NOTE: You can safely ignore the above warning unless this call should not 
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't 
mean to enforce the call.  See 
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#knowing-when-to-expect
 for details.

GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: disconnected(0x7fff12024248)
NOTE: You can safely ignore the above warning unless this call should not 
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't 
mean to enforce the call.  See 
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#knowing-when-to-expect
 for details.

GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: registered(0x7fff12024248, @0x7fe5ec001740 
622fd6fc-f57a-4657-8d79-d30efb8f8e66-, @0x7fe5ec002130 id: 
"f935fe81-68e3-4dbd-b258-6c216e9bb4c7"
Ip: 3492307904
Port: 37105
Pid: "master@192.99.40.208:37105"
Hostname: "gru1.hw.ca1.mesosphere.com"
Version: "1.6.0"
Address {
  hostname: "gru1.hw.ca1.mesosphere.com"
  ip: "192.99.40.208"
  port: 37105
}
Capabilities {
  type: AGENT_UPDATE
}
)
NOTE: You can safely ignore the above warning unless this call should not 
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't 
mean to enforce the call.  See 
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#knowing-when-to-expect
 for details.

GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: offerRescinded(0x7fff12024248, @0x7fe5ec0015a8 
f935fe81-68e3-4dbd-b258-6c216e9bb4c7-O0)
NOTE: You can safely ignore the above warning unless this call should not 
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't 
mean to enforce the call.  See 
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#knowing-when-to-expect
 for details.
../src/tests/master_tests.cpp:8916: Failure


src/tests/master_tests.cpp
Lines 8804 (patched)


nit: get rid of this line.



src/tests/master_tests.cpp
Lines 8872 (patched)


Since with only assertions it becomes hard to recognize what is being 
tested, let's expect here.



src/tests/master_tests.cpp
Lines 8942 (patched)


We could expect here for self-documentation.


- Benjamin Bannier


On Jan. 18, 2018, 3:11 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65045/
> ---
> 
> (Updated Jan. 18, 2018, 3:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tested correct operation handling during master failover.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp d01f3fbdd688ddd31fb0c777f973928f5b5fa5e7 
> 
> 
> Diff: https://reviews.apache.org/r/65045/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 65253: Avoided dropping events on the master operator API stream.

[Greg Mann]

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

Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

The master's `Subscribers::send()` call path previously accessed
event-related master state after an asynchronous call. Thus, if
that state changed after the call to `Subscribers::send()` but
before the event was actually sent, messages could be dropped.

This patch updates that call path to capture a shared copy of
event-related metadata so that the asynchronous calls have a
consistent view of the master's state at the time of the event.


Diffs
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Benjamin Bannier]

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




include/mesos/agent/agent.proto
Lines 72 (patched)


I'd suggest to sort this right before `GET_TASKS`.



include/mesos/agent/agent.proto
Lines 422 (patched)


Ditto.



include/mesos/agent/agent.proto
Lines 549 (patched)


Ditto.



include/mesos/agent/agent.proto
Lines 615 (patched)


Let's sort this right before `get_tasks`.



include/mesos/master/master.proto
Lines 67 (patched)


Let's sort this before `GET_TASKS`.



include/mesos/master/master.proto
Lines 266 (patched)


Ditto.



include/mesos/master/master.proto
Lines 461 (patched)


Remove one line.



include/mesos/master/master.proto
Lines 462 (patched)


Ditto.



include/mesos/master/master.proto
Lines 512 (patched)


Ditto.



include/mesos/v1/agent/agent.proto
Lines 72 (patched)


Ditto.



include/mesos/v1/agent/agent.proto
Lines 422 (patched)


Ditto.



include/mesos/v1/agent/agent.proto
Lines 549 (patched)


Ditto.



include/mesos/v1/agent/agent.proto
Lines 615 (patched)


Ditto.



include/mesos/v1/master/master.proto
Lines 65 (patched)


Ditto.



include/mesos/v1/master/master.proto
Lines 264 (patched)


Ditto.



include/mesos/v1/master/master.proto
Lines 457 (patched)


Ditto.



include/mesos/v1/master/master.proto
Lines 507 (patched)


Ditto.



src/master/http.cpp
Lines 3761 (patched)


This seems inconsistent with other handlers.



src/tests/api_tests.cpp
Lines 1011 (patched)


Let's mention here that this is 5s.



src/tests/api_tests.cpp
Lines 1076 (patched)


We need to assert this to not run into a false negative below.



src/tests/api_tests.cpp
Lines 1134 (patched)


Let's expect exactly one operation here.



src/tests/api_tests.cpp
Lines 6277 (patched)


Ditto.



src/tests/api_tests.cpp
Lines 6340 (patched)


Ditto.



src/tests/api_tests.cpp
Lines 6398 (patched)


Ditto.


- Benjamin Bannier


On Jan. 22, 2018, 3:02 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65044/
> ---
> 
> (Updated Jan. 22, 2018, 3:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_OPERATIONS' call lists all operations known to master or agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
>   include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
>   include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
>   include/mesos/v1/master/master.proto 
> 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
>   src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
>   src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
>   src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
>   src/tests/api_tests.cpp bdacc30be4dc8656a41a0c47d0e350d48e59ad94 
> 
> 
> Diff: https://reviews.apache.org/r/65044/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65189: Displayed all needed commands in apply-reviews script dry-run mode.

[Armand Grillet]

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


Ship it!




Ship It!

- Armand Grillet


On Jan. 22, 2018, 1:58 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65189/
> ---
> 
> (Updated Jan. 22, 2018, 1:58 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Kevin Klues.
> 
> 
> Bugs: MESOS-8447
> https://issues.apache.org/jira/browse/MESOS-8447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a shell command to create a file containing the commit
> message for dry-run mode. To minimize platform differences we use
> `printf` instead of `echo`. We output the message on a single line
> for compactness.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py 0ef28cb02bc65acfeb7ea6808f74e1620a8a85c4 
> 
> 
> Diff: https://reviews.apache.org/r/65189/diff/2/
> 
> 
> Testing
> ---
> 
> Redirected output of `apply-reviews.py --dry-run` to a file and successfully 
> applied its output with `sh`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65263: Updated `SlaveRecoveryTest.RecoverCompletedExecutor` to verify gc.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65263 was successfully built and tested.

Reviews applied: `['65263']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65263

- Mesos Reviewbot Windows


On Jan. 22, 2018, 2:21 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65263/
> ---
> 
> (Updated Jan. 22, 2018, 2:21 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8462
> https://issues.apache.org/jira/browse/MESOS-8462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the test `SlaveRecoveryTest.RecoverCompletedExecutor`, when the
> completed executor is recovered, verify its work and meta directories
> gc'ed successfully.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 6d8a57138903637d3cf9e5b19a466961d2c120d7 
> 
> 
> Diff: https://reviews.apache.org/r/65263/diff/1/
> 
> 
> Testing
> ---
> 
> Manually ran this test repeatedly (100 times).
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65189: Displayed all needed commands in apply-reviews script dry-run mode.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65189 was successfully built and tested.

Reviews applied: `['65189']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65189

- Mesos Reviewbot Windows


On Jan. 22, 2018, 7:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65189/
> ---
> 
> (Updated Jan. 22, 2018, 7:58 a.m.)
> 
> 
> Review request for mesos, Armand Grillet and Kevin Klues.
> 
> 
> Bugs: MESOS-8447
> https://issues.apache.org/jira/browse/MESOS-8447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a shell command to create a file containing the commit
> message for dry-run mode. To minimize platform differences we use
> `printf` instead of `echo`. We output the message on a single line
> for compactness.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py 0ef28cb02bc65acfeb7ea6808f74e1620a8a85c4 
> 
> 
> Diff: https://reviews.apache.org/r/65189/diff/2/
> 
> 
> Testing
> ---
> 
> Redirected output of `apply-reviews.py --dry-run` to a file and successfully 
> applied its output with `sh`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65261: Fixed connection refused error in IOSwitchboard for unix socket.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65261 was successfully built and tested.

Reviews applied: `['65261']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65261

- Mesos Reviewbot Windows


On Jan. 22, 2018, 2 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65261/
> ---
> 
> (Updated Jan. 22, 2018, 2 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jie Yu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7742
> https://issues.apache.org/jira/browse/MESOS-7742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Once IOSwitchboard's unix socket appears in the file system, the agent
> can try to connect to it. If the agent attempts to connect to the
> socket before `listen` has been called, then it gets connection refused
> error. To address this, create a unix socket using a temporary name,
> then initialize it, then rename the socket with the original path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 45b615be60af9af839309011eea85912c01ff451 
>   src/slave/containerizer/mesos/paths.hpp 
> 65830a153f765c74bb0399bca9b2941390eeb531 
>   src/slave/containerizer/mesos/paths.cpp 
> 0df4cf5f50aee94a2758ce885389fb488a85fdbd 
> 
> 
> Diff: https://reviews.apache.org/r/65261/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Jan Schlicht]

> On Jan. 17, 2018, 10:18 a.m., Benjamin Bannier wrote:
> > src/common/protobuf_utils.cpp
> > Lines 1288-1290 (patched)
> > 
> >
> > Once we have settled where to transport this information we should 
> > create a ticket for adding authorization to this under 
> > https://issues.apache.org/jira/browse/MESOS-8374.

I've created MESOS-8473 and mention it in the `GET_OPERATIONS` HTTP handlers.


- Jan


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


On Jan. 22, 2018, 3:02 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65044/
> ---
> 
> (Updated Jan. 22, 2018, 3:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_OPERATIONS' call lists all operations known to master or agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
>   include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
>   include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
>   include/mesos/v1/master/master.proto 
> 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
>   src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
>   src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
>   src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
>   src/tests/api_tests.cpp bdacc30be4dc8656a41a0c47d0e350d48e59ad94 
> 
> 
> Diff: https://reviews.apache.org/r/65044/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Jan Schlicht]

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

(Updated Jan. 22, 2018, 3:02 p.m.)


Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jie 
Yu.


Changes
---

Mentioned MESOS-8473.


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


Repository: mesos


Description
---

The 'GET_OPERATIONS' call lists all operations known to master or agent.


Diffs (updated)
-

  include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
  include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
  include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
  include/mesos/v1/master/master.proto 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
  src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
  src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
  src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
  src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
  src/tests/api_tests.cpp bdacc30be4dc8656a41a0c47d0e350d48e59ad94 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 65189: Displayed all needed commands in apply-reviews script dry-run mode.

[Benjamin Bannier]

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

(Updated Jan. 22, 2018, 2:58 p.m.)


Review request for mesos, Armand Grillet and Kevin Klues.


Changes
---

Renamed variable.


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


Repository: mesos


Description
---

This patch adds a shell command to create a file containing the commit
message for dry-run mode. To minimize platform differences we use
`printf` instead of `echo`. We output the message on a single line
for compactness.


Diffs (updated)
-

  support/apply-reviews.py 0ef28cb02bc65acfeb7ea6808f74e1620a8a85c4 


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

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


Testing
---

Redirected output of `apply-reviews.py --dry-run` to a file and successfully 
applied its output with `sh`.


Thanks,

Benjamin Bannier

Re: Review Request 64386: Refactored health checks to take in executor information.

[Alexander Rukletsov]

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



Akash — good stuff, I like the direction it is going. Let's do another 
iteration to further clean up the library interface.


src/checks/checker.hpp
Lines 69-70 (original), 73-74 (patched)


These guys can now be part of `MesosRuntimeInfo` : )



src/checks/checker.hpp
Lines 100-102 (original), 107-109 (patched)


And these guys — part of `UCRuntimeInfo`!



src/checks/checker_process.hpp
Lines 40-47 (patched)


What meaning do you put in the difference between `UCR` and `MESOS`? For 
me, the difference is whether the checker library talks to the task directly or 
via a container runtime; as a consequence, different types of command checks is 
used.



src/checks/checker_process.hpp
Lines 40-69 (patched)


These types remind me of protobufs. I do not envision a need to make them 
protobufs in the nearest future. Here is what comes to my mind instead:
```
class RuntimeOptions
{
public:
  enum class Type
  {
  MESOS,
  DOCKER,
  UCR
  };
  
private:
  const Type type;
};

class MesosRuntimeOptions: public RuntimeOptions
{
public:
  MesosRuntimeOptions(...): RuntimeOptions(MESOS), ...;

private:
  const Option taskPid;
  const std::vector namespaces;
};

class DockerRuntimeOptions: public RuntimeOptions
{
public:
  DockerRuntimeOptions(...): RuntimeOptions(Docker), ...;
  
private:
  const std::string dockerPath;
  const std::string socketName;
  const std::string containerName;
}
```
What do you think? I would also suggest to declare these types in a 
separate file to minimize dependencies and hence compilation time. Also, this 
should likely be a separate patch ; )



src/checks/checker_process.cpp
Lines 467-473 (patched)


This looks good to me, but it is not exactly what the original code is 
doing. Can you please do this change in a separate patch? It is hard to spot 
such changes (and the bisect or revert if necessary), when they are combined 
with moving code around. Thanks!



src/launcher/default_executor.cpp
Lines 551 (patched)


I'd argue, you should use `UCR` here: the library relies on the agent 
(which contains UCR) for command health checks.



src/launcher/default_executor.cpp
Lines 565-567 (original), 568-570 (patched)


And here is the beauty of your approach: these guys should go into 
`UCRContainerInfo` runtime!



src/launcher/executor.cpp
Lines 653-658 (patched)


How is this used in the checker library at the moment? Do you distinguish 
there between Mesos and UCR?


- Alexander Rukletsov


On Jan. 17, 2018, 12:10 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64386/
> ---
> 
> (Updated Jan. 17, 2018, 12:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added information about the calling executor to the health check
> library. After the refactoring, the command health check code originally
> in the docker executor is now in the health check library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/64386/diff/8/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/64387/
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Review Request 65263: Updated `SlaveRecoveryTest.RecoverCompletedExecutor` to verify gc.

[Qian Zhang]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

In the test `SlaveRecoveryTest.RecoverCompletedExecutor`, when the
completed executor is recovered, verify its work and meta directories
gc'ed successfully.


Diffs
-

  src/tests/slave_recovery_tests.cpp 6d8a57138903637d3cf9e5b19a466961d2c120d7 


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


Testing
---

Manually ran this test repeatedly (100 times).


Thanks,

Qian Zhang

Re: Review Request 65215: Updated mesos-tidy setup for upgraded Boost version.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 62161.

Failed command: `python.exe .\support\apply-reviews.py -n -r 62161`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65215

Relevant logs:

- 
[apply-review-62161-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65215/logs/apply-review-62161-stdout.log):

```
error: missing binary patch data for '3rdparty/boost-1.65.0.tar.gz'
error: binary patch does not apply to '3rdparty/boost-1.65.0.tar.gz'
error: 3rdparty/boost-1.65.0.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On Jan. 22, 2018, 12:58 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65215/
> ---
> 
> (Updated Jan. 22, 2018, 12:58 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a previous commit we updated the bundled Boost version. This patch
> updates the mesos-tidy setup to make sure we build the correct bundled
> Boost version when creating analysis prerequisites.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh ef3e2a817ce704ff3f08734da7457cd87d95 
> 
> 
> Diff: https://reviews.apache.org/r/65215/diff/1/
> 
> 
> Testing
> ---
> 
> Built an updated tidy docker image
> 
> % cd support/mesos-tidy && docker build -t wip .
> 
> Ran a modified script to use updated docker image on top of r/62447
> 
> % ./support/mesos-tidy_local.sh
> 
> No new violations are reported.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 65261: Fixed connection refused error in IOSwitchboard for unix socket.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Jie Yu, and Kevin Klues.


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


Repository: mesos


Description
---

Once IOSwitchboard's unix socket appears in the file system, the agent
can try to connect to it. If the agent attempts to connect to the
socket before `listen` has been called, then it gets connection refused
error. To address this, create a unix socket using a temporary name,
then initialize it, then rename the socket with the original path.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
45b615be60af9af839309011eea85912c01ff451 
  src/slave/containerizer/mesos/paths.hpp 
65830a153f765c74bb0399bca9b2941390eeb531 
  src/slave/containerizer/mesos/paths.cpp 
0df4cf5f50aee94a2758ce885389fb488a85fdbd 


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


Testing
---

make check (Fedora 25)
internal CI


Thanks,

Andrei Budnik

Review Request 65215: Updated mesos-tidy setup for upgraded Boost version.

[Benjamin Bannier]

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

Review request for mesos, Benno Evers and Michael Park.


Repository: mesos


Description
---

In a previous commit we updated the bundled Boost version. This patch
updates the mesos-tidy setup to make sure we build the correct bundled
Boost version when creating analysis prerequisites.


Diffs
-

  support/mesos-tidy/entrypoint.sh ef3e2a817ce704ff3f08734da7457cd87d95 


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


Testing
---

Built an updated tidy docker image

% cd support/mesos-tidy && docker build -t wip .

Ran a modified script to use updated docker image on top of r/62447

% ./support/mesos-tidy_local.sh

No new violations are reported.


Thanks,

Benjamin Bannier

Re: Review Request 65203: Updated the CHANGELOG for 1.5.0 release.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Jan. 20, 2018, 2:01 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65203/
> ---
> 
> (Updated Jan. 20, 2018, 2:01 a.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Andrew Schwartzmeyer, 
> Benjamin Mahler, Chun-Hung Hsiao, Gaston Kleiman, Greg Mann, Ilya Pronin, Jie 
> Yu, James Peach, Kapil Arya, Joseph Wu, Kevin Klues, Neil Conway, Qian Zhang, 
> Till Toenshoff, Vinod Kone, Jiang Yan Xu, and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the CHANGELOG for 1.5.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 078c5d64100d81ce84dbdff4055cdda465e19011 
> 
> 
> Diff: https://reviews.apache.org/r/65203/diff/2/
> 
> 
> Testing
> ---
> 
> N/A.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 65189: Displayed all needed commands in apply-reviews script dry-run mode.

[Armand Grillet]

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




support/apply-reviews.py
Lines 275 (patched)


Can you rename `message` to something else as we already have `with 
open(message_file, 'w') as message:`.


- Armand Grillet


On Jan. 17, 2018, 12:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65189/
> ---
> 
> (Updated Jan. 17, 2018, 12:22 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Kevin Klues.
> 
> 
> Bugs: MESOS-8447
> https://issues.apache.org/jira/browse/MESOS-8447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a shell command to create a file containing the commit
> message for dry-run mode. To minimize platform differences we use
> `printf` instead of `echo`. We output the message on a single line
> for compactness.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py 0ef28cb02bc65acfeb7ea6808f74e1620a8a85c4 
> 
> 
> Diff: https://reviews.apache.org/r/65189/diff/1/
> 
> 
> Testing
> ---
> 
> Redirected output of `apply-reviews.py --dry-run` to a file and successfully 
> applied its output with `sh`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>