Re: Review Request 61570: Added the `kill()` function to the containerizer interface.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 2463 (patched)


I don't like this semantics. The signal here can be any signal, not 
necessarily a SIGKILL/SIGTERM. For instance, SIGUSR1, SIGALARM, etc., which 
might not be terminal signal.

Any reason why we want to destroy the container here?


- Jie Yu


On Aug. 14, 2017, 11:09 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61570/
> ---
> 
> (Updated Aug. 14, 2017, 11:09 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This would be used by now on for killing a container by sending
> a signal to it similar to the linux equivalent `kill()` system call.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0954ed6175e4c8f963bf371e54e0f9ffe7bc9c1c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> fd586306f7a6d3d2cdb58481f0ef4906de8d0f88 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ff192bb085f3554ad1b1f20fb73bf827bf04ef13 
> 
> 
> Diff: https://reviews.apache.org/r/61570/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61693: Fixed a bug around the `kill()` not removing active containers.

[Jie Yu]

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



Also, could you please paste the commit sha and link the ticket?

- Jie Yu


On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> ---
> 
> (Updated Aug. 16, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> f1a9c3d08b408aa61198f4042b9274df88b789ea 
> 
> 
> Diff: https://reviews.apache.org/r/61693/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (tests pass)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61693: Fixed a bug around the `kill()` not removing active containers.

[Jie Yu]

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




src/slave/containerizer/composing.cpp
Lines 656-657 (patched)


Hum, this does not look like the right solution to me. How is that 
different from user manually sending a `SIGKILL` to the leading process of the 
container, triggering a reap? Don't you have the same problem in that case? Why 
we do a special case here?

A reap event should trigger a destroy of the container, leading to `wait` 
being returned. The component waiting for the container to terminate will then 
call destroy, causing the corresponding container in the composing 
containerizer being cleaned up.


- Jie Yu


On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> ---
> 
> (Updated Aug. 16, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> f1a9c3d08b408aa61198f4042b9274df88b789ea 
> 
> 
> Diff: https://reviews.apache.org/r/61693/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (tests pass)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61272: Added a MockResourceProvider.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 15, 2017, 12:05 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61272/
> ---
> 
> (Updated Aug. 15, 2017, 12:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a MockResourceProvider.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 6f06261d81870b923b7053daf8205327c4ac6a45 
> 
> 
> Diff: https://reviews.apache.org/r/61272/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61294: Imported `protobuf` library.

[Joseph Wu]

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


Ship it!




Hm... I might squash all the reviews that change libraries to being imported 
(into https://reviews.apache.org/r/61291/ ).  I'll come to a decision once I've 
gotten through all of said reviews.

Anyway, LGTM.  But needs some extra whitespace.  And some of the style isn't 
consistent (I'll fixup).

- Joseph Wu


On Aug. 1, 2017, 5:54 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61294/
> ---
> 
> (Updated Aug. 1, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Imported `protobuf` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt f4feaf90b4750338cfa2127524a5e6556a86d063 
> 
> 
> Diff: https://reviews.apache.org/r/61294/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 61602: Fixed mesos containerizer to support docker image WORKDIR missing.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 17, 2017, 3:33 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61602/
> ---
> 
> (Updated Aug. 17, 2017, 3:33 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-7652
> https://issues.apache.org/jira/browse/MESOS-7652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some docker image may have 'WORKDIR' set in its manifest but that
> 'WORKDIR' does not exist in the image rootfs (e.g., the workdir
> is removed in the following dockerfile).
> 
> From the reference of dockerfile, "If the WORKDIR doesn’t exist,
> it will be created even if it’s not used in any subsequent
> Dockerfile instruction". So we should create the working directory
> if it does not exist in the image's rootfs.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 8e662931697a2f20e0ac1e80a2911b96f646b5af 
> 
> 
> Diff: https://reviews.apache.org/r/61602/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> Manually tested using 'quay.io/spinnaker/front50:master' docker image.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61602: Fixed mesos containerizer to support docker image WORKDIR missing.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61602]

Failed command: python support/apply-reviews.py -n -r 61602

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
  File "support/apply-reviews.py", line 261, in commit_patch
message.write(data['message'])
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2019' in position 
295: ordinal not in range(128)

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/240/console

- Mesos Reviewbot Windows


On Aug. 16, 2017, 8:33 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61602/
> ---
> 
> (Updated Aug. 16, 2017, 8:33 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-7652
> https://issues.apache.org/jira/browse/MESOS-7652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some docker image may have 'WORKDIR' set in its manifest but that
> 'WORKDIR' does not exist in the image rootfs (e.g., the workdir
> is removed in the following dockerfile).
> 
> From the reference of dockerfile, "If the WORKDIR doesn’t exist,
> it will be created even if it’s not used in any subsequent
> Dockerfile instruction". So we should create the working directory
> if it does not exist in the image's rootfs.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 8e662931697a2f20e0ac1e80a2911b96f646b5af 
> 
> 
> Diff: https://reviews.apache.org/r/61602/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> Manually tested using 'quay.io/spinnaker/front50:master' docker image.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61602: Fixed mesos containerizer to support docker image WORKDIR missing.

[Gilbert Song]

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

(Updated Aug. 16, 2017, 8:33 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Qian Zhang.


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


Repository: mesos


Description
---

Some docker image may have 'WORKDIR' set in its manifest but that
'WORKDIR' does not exist in the image rootfs (e.g., the workdir
is removed in the following dockerfile).

>From the reference of dockerfile, "If the WORKDIR doesn’t exist,
it will be created even if it’s not used in any subsequent
Dockerfile instruction". So we should create the working directory
if it does not exist in the image's rootfs.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 
8e662931697a2f20e0ac1e80a2911b96f646b5af 


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

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


Testing
---

make

Manually tested using 'quay.io/spinnaker/front50:master' docker image.


Thanks,

Gilbert Song

Re: Review Request 61693: Fixed a bug around the `kill()` not removing active containers.

[Gilbert Song]

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


Fix it, then Ship it!





src/slave/containerizer/composing.cpp
Lines 654-655 (patched)


Could we add a `TODO` here later?

TODO: the `destroy` callback would result in extra unnecessary 
containerizer->destroy() call, which can be improved.


- Gilbert Song


On Aug. 16, 2017, 10:27 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> ---
> 
> (Updated Aug. 16, 2017, 10:27 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> f1a9c3d08b408aa61198f4042b9274df88b789ea 
> 
> 
> Diff: https://reviews.apache.org/r/61693/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (tests pass)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61693: Fixed a bug around the `kill()` not removing active containers.

[Vinod Kone]

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


Ship it!




LGTM assuming `destroy` is idempotent.

- Vinod Kone


On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> ---
> 
> (Updated Aug. 16, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> f1a9c3d08b408aa61198f4042b9274df88b789ea 
> 
> 
> Diff: https://reviews.apache.org/r/61693/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (tests pass)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61654: Added a stream ID to resource provider manager connections.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 15, 2017, 12:06 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61654/
> ---
> 
> (Updated Aug. 15, 2017, 12:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to resolve problems with resource providers that erroneously
> try to 'SUBSCRIBE' multiple times a unique stream ID is associated with
> a connected resource provider. Resource providers are expected to send
> this stream ID with every call that isn't a 'SUBSCRIBE' call.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
> 
> 
> Diff: https://reviews.apache.org/r/61654/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61705: Added a mock CSI plugin and a unit test for CSI client classes.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61705, 61704, 61703, 61600]

Failed command: python support/apply-reviews.py -n -r 61703

Error:
error: missing binary patch data for '3rdparty/csi-0.1.0.tar.gz'
error: binary patch does not apply to '3rdparty/csi-0.1.0.tar.gz'
error: 3rdparty/csi-0.1.0.tar.gz: patch does not apply

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/240/console

- Mesos Reviewbot Windows


On Aug. 16, 2017, 4:13 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61705/
> ---
> 
> (Updated Aug. 16, 2017, 4:13 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: mesos-7491
> https://issues.apache.org/jira/browse/mesos-7491
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The mock plugin simply starts the `Identity`, `Controller` and `Node`
> CSI services and return a success with an empty response protocol buffer
> for each RPC. The unit test verifies that each method in the CSI client
> classes makes the corresponding RPC call through the gRPC interface in
> libprocess.
> 
> 
> Diffs
> -
> 
>   src/tests/csi_client_tests.cpp PRE-CREATION 
>   src/tests/mock_csi_plugin.hpp PRE-CREATION 
>   src/tests/mock_csi_plugin.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61705/diff/1/
> 
> 
> Testing
> ---
> 
> Tests described in r/61706.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 61705: Added a mock CSI plugin and a unit test for CSI client classes.

[Chun-Hung Hsiao]

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

Review request for mesos and Jie Yu.


Bugs: mesos-7491
https://issues.apache.org/jira/browse/mesos-7491


Repository: mesos


Description
---

The mock plugin simply starts the `Identity`, `Controller` and `Node`
CSI services and return a success with an empty response protocol buffer
for each RPC. The unit test verifies that each method in the CSI client
classes makes the corresponding RPC call through the gRPC interface in
libprocess.


Diffs
-

  src/tests/csi_client_tests.cpp PRE-CREATION 
  src/tests/mock_csi_plugin.hpp PRE-CREATION 
  src/tests/mock_csi_plugin.cpp PRE-CREATION 


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


Testing
---

Tests described in r/61706.


Thanks,

Chun-Hung Hsiao

Re: Review Request 61704: Added CSI client classes to talk to CSI plugins.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61704, 61703, 61600]

Failed command: python support/apply-reviews.py -n -r 61703

Error:
error: missing binary patch data for '3rdparty/csi-0.1.0.tar.gz'
error: binary patch does not apply to '3rdparty/csi-0.1.0.tar.gz'
error: 3rdparty/csi-0.1.0.tar.gz: patch does not apply

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/240/console

- Mesos Reviewbot Windows


On Aug. 16, 2017, 11:08 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61704/
> ---
> 
> (Updated Aug. 16, 2017, 11:08 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: mesos-7491
> https://issues.apache.org/jira/browse/mesos-7491
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CSI client classes to talk to CSI plugins.
> 
> 
> Diffs
> -
> 
>   include/mesos/csi/csi.hpp PRE-CREATION 
>   src/csi/csi.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61704/diff/1/
> 
> 
> Testing
> ---
> 
> Tests described in r/61706.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 61704: Added CSI client classes to talk to CSI plugins.

[Chun-Hung Hsiao]

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

Review request for mesos and Jie Yu.


Bugs: mesos-7491
https://issues.apache.org/jira/browse/mesos-7491


Repository: mesos


Description
---

Added CSI client classes to talk to CSI plugins.


Diffs
-

  include/mesos/csi/csi.hpp PRE-CREATION 
  src/csi/csi.cpp PRE-CREATION 


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


Testing
---

Tests described in r/61706.


Thanks,

Chun-Hung Hsiao

Review Request 61703: Bundled CSI v0.1.0 into 3rdparty libraries.

[Chun-Hung Hsiao]

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

Review request for mesos and Jie Yu.


Bugs: mesos-7491
https://issues.apache.org/jira/browse/mesos-7491


Repository: mesos


Description
---

The bundled package is generated wih the following command:

git clone https://github.com/container-storage-interface/spec.git csi
cd csi && git archive --prefix=csi-0.1.0/ 6606242 |
  gzip > csi-0.1.0.tar.gz


Diffs
-

  3rdparty/csi-0.1.0.tar.gz PRE-CREATION 


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


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Greg Mann]

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




src/checks/checker_process.cpp
Lines 143-144 (patched)


"returns the merged stdout and stderr": is this true? Looks like you only 
use STDOUT type events here?



src/checks/checker_process.cpp
Lines 145 (patched)


s/It/This function/



src/checks/checker_process.cpp
Lines 153 (patched)


So this fuction expects a stream with ContentType of protobuf; could you 
call this out in the comment above the function declaration?



src/checks/checker_process.cpp
Lines 543-545 (original), 581-583 (patched)


Could you extend this comment with TODO explaining why we want to switch to 
a streaming response (for logging of hung checks), with a reference to a 
relevant JIRA?


- Greg Mann


On Aug. 16, 2017, 8:17 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 16, 2017, 8:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/2/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61697]

Failed command: python support/apply-reviews.py -n -r 61697

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
  File "support/apply-reviews.py", line 268, in commit_patch
shell(cmd, options['dry_run'])
  File "support/apply-reviews.py", line 144, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
  File "C:\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
  File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 
25: ordinal not in range(128)

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/239/console

- Mesos Reviewbot Windows


On Aug. 16, 2017, 8:17 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 16, 2017, 8:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/2/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Gastón Kleiman]

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

(Updated Aug. 16, 2017, 8:17 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


Changes
---

Style changes.


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


Repository: mesos


Description
---

This patch makes the default executor include the output of the COMMAND
(health) checks in its logs.


Diffs (updated)
-

  src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 


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

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


Testing
---

Manual tests.


Thanks,

Gastón Kleiman

Re: Review Request 61697: Included nested command checks output in the executor logs.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61697]

Failed command: python support/apply-reviews.py -n -r 61697

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
  File "support/apply-reviews.py", line 268, in commit_patch
shell(cmd, options['dry_run'])
  File "support/apply-reviews.py", line 144, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
  File "C:\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
  File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 
25: ordinal not in range(128)

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/238/console

- Mesos Reviewbot Windows


On Aug. 16, 2017, 8:11 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> ---
> 
> (Updated Aug. 16, 2017, 8:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 
> 
> 
> Diff: https://reviews.apache.org/r/61697/diff/1/
> 
> 
> Testing
> ---
> 
> Manual tests.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 61697: Included nested command checks output in the executor logs.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and Vinod Kone.


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


Repository: mesos


Description
---

This patch makes the default executor include the output of the COMMAND
(health) checks in its logs.


Diffs
-

  src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e 


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


Testing
---

Manual tests.


Thanks,

Gastón Kleiman

Re: Review Request 61575: Added a test for verifying signal escalation on the default executor.

[Mesos Reviewbot Windows]

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



Failed to apply patch!

Reviews applied: [61569, 61570, 61571, 61572, 61573, 61668, 61574, 61575]

Logs available here: http://104.210.40.105/logs/master/61575

- Mesos Reviewbot Windows


On Aug. 16, 2017, 12:14 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61575/
> ---
> 
> (Updated Aug. 16, 2017, 12:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test uses the kill policy helper and blocks the SIGTERM signal.
> 
> Review: https://reviews.apache.org/r/61575
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> afe0afabf784fb65eb833beadd3c584722c321e1 
>   src/tests/kill_policy_test_helper.hpp 
> 29651102ec46b477e6e797c6e6bdef5b10afa665 
>   src/tests/kill_policy_test_helper.cpp 
> a1880595ff015475f1ba49437d49f7397da19422 
> 
> 
> Diff: https://reviews.apache.org/r/61575/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61693: Fixed a bug around the `kill()` not removing active containers.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61693]

Logs available here: http://104.210.40.105/logs/master/61693

- Mesos Reviewbot Windows


On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> ---
> 
> (Updated Aug. 16, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> f1a9c3d08b408aa61198f4042b9274df88b789ea 
> 
> 
> Diff: https://reviews.apache.org/r/61693/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (tests pass)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61575: Added a test for verifying signal escalation on the default executor.

[Anand Mazumdar]

> On Aug. 14, 2017, 7:04 p.m., Gastón Kleiman wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 600 (patched)
> > 
> >
> > s/EXPECT_NE(0, 
> > offers->offers().size());/EXPECT_FALSE(offers->offers().empty)());

I used `ASSERT_NE` instead.


> On Aug. 14, 2017, 7:04 p.m., Gastón Kleiman wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 613-661 (patched)
> > 
> >
> > What do you think about the following shorter (and IMHO more readable) 
> > approach?
> > 
> > ```
> >   v1::Offer::Operation launchGroup = v1::LAUNCH_GROUP(
> >   executorInfo,
> >   v1::createTaskGroupInfo({taskInfo}));
> > 
> >   Future update1;
> >   EXPECT_CALL(*scheduler, update(_, _))
> > .WillOnce(DoAll(
> > FutureArg<1>(),
> > v1::scheduler::SendAcknowledge(
> > frameworkId,
> > offer.agent_id(;
> > 
> >   mesos.send(v1::createCallAccept(
> >   frameworkId,
> >   offer,
> >   {reserve, create, launchGroup}));
> > ```

We need to do a sweep of the file before using these new helpers first to be 
consistent within this file.


- Anand


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


On Aug. 16, 2017, 7:14 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61575/
> ---
> 
> (Updated Aug. 16, 2017, 7:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7879
> https://issues.apache.org/jira/browse/MESOS-7879
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test uses the kill policy helper and blocks the SIGTERM signal.
> 
> Review: https://reviews.apache.org/r/61575
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> afe0afabf784fb65eb833beadd3c584722c321e1 
>   src/tests/kill_policy_test_helper.hpp 
> 29651102ec46b477e6e797c6e6bdef5b10afa665 
>   src/tests/kill_policy_test_helper.cpp 
> a1880595ff015475f1ba49437d49f7397da19422 
> 
> 
> Diff: https://reviews.apache.org/r/61575/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 61575: Added a test for verifying signal escalation on the default executor.

[Anand Mazumdar]

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

(Updated Aug. 16, 2017, 7:14 p.m.)


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


Changes
---

Review comments.


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


Repository: mesos


Description (updated)
---

This test uses the kill policy helper and blocks the SIGTERM signal.

Review: https://reviews.apache.org/r/61575


Diffs (updated)
-

  src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1 
  src/tests/kill_policy_test_helper.hpp 
29651102ec46b477e6e797c6e6bdef5b10afa665 
  src/tests/kill_policy_test_helper.cpp 
a1880595ff015475f1ba49437d49f7397da19422 


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

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 61682: Fixed usage of 'ATOMIC_FLAG_INIT'.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Aug. 16, 2017, 8:35 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61682/
> ---
> 
> (Updated Aug. 16, 2017, 8:35 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The C++ standard guarantees that the macro 'ATOMIC_FLAG_INIT' is
> usable for initialization of atomic variables; its use in other
> context is unspecified.
> 
> libcxx defines 'ATOMIC_FLAG_INIT' as '{false}' which when used in an
> initializer list yields a clang warning,
> 
> warning: braces around scalar initializer [-Wbraced-scalar-init]
> lock(ATOMIC_FLAG_INIT)
> 
> This patch prevents such a warning by initializing 'atomic_flag'
> member variables in the class declaration instead of in an initializer
> list.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/grpc.hpp 
> 91c1cd9d6e3610e9978fae083fa75d781defa175 
>   3rdparty/libprocess/src/grpc.cpp 3ba5bc5415d55d7be044cfc0f4e240358cb3f4e8 
> 
> 
> Diff: https://reviews.apache.org/r/61682/diff/1/
> 
> 
> Testing
> ---
> 
> With clang-6.0.0: `./configure --enable-grpc && make -C 3rdparty/libprocess 
> tests` emits no compiler warning.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61693: Fixed a bug around the `kill()` not removing active containers.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61693]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> ---
> 
> (Updated Aug. 16, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> f1a9c3d08b408aa61198f4042b9274df88b789ea 
> 
> 
> Diff: https://reviews.apache.org/r/61693/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (tests pass)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 61693: Fixed a bug around the `kill()` not removing active containers.

[Anand Mazumdar]

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

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


Repository: mesos


Description
---

The composing containerizer relies on users invoking `destroy()`
for removing the container from the list of active containers.
However, when using `kill()` the relevant containerizer just
reaps the container without notifying the composing containerizer
i.e., there is no `destroy()` invocation from the user. This change
chains on the `wait()` future to invoke `destroy()` that
eventually does the cleanup.


Diffs
-

  src/slave/containerizer/composing.cpp 
f1a9c3d08b408aa61198f4042b9274df88b789ea 


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


Testing
---

sudo make check (tests pass)


Thanks,

Anand Mazumdar

Re: Review Request 61690: Added `--zk_session_timeout` flag for agent.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61689, 61690]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 16, 2017, 1:58 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61690/
> ---
> 
> (Updated Aug. 16, 2017, 1:58 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7895
> https://issues.apache.org/jira/browse/MESOS-7895
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently `ZooKeeperMasterDetector` in agent uses default session
> timeout and there's no way to configure it. This patch introduces
> `--zk_session_timeout` flag for agent similar to the one in master.
> 
> 
> Diffs
> -
> 
>   src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
>   src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 
>   src/slave/main.cpp a4a8ced6825ece1b25003824d987ff83fb799ad7 
> 
> 
> Diff: https://reviews.apache.org/r/61690/diff/1/
> 
> 
> Testing
> ---
> 
> Ran `make check`.
> 
> Manually verified that ZK session negotiates 20 secs timeout instead of 10 
> secs when the agent is started with `--zk_session_timeout=20secs`.
> ```
> 2017-08-15 17:50:32,065:7763(0x7051c000):ZOO_INFO@check_events@1728: 
> initiated connection to server [192.168.99.100:2181]
> 2017-08-15 17:50:32,069:7763(0x7051c000):ZOO_INFO@check_events@1775: 
> session establishment complete on server [192.168.99.100:2181], 
> sessionId=0x15de6c7f8920001, negotiated timeout=2
> ```
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 61285: Added library linkage and suffix variables.

[Joseph Wu]

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


Ship it!




LGTM.

- Joseph Wu


On Aug. 1, 2017, 5:53 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61285/
> ---
> 
> (Updated Aug. 1, 2017, 5:53 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-3576
> https://issues.apache.org/jira/browse/MESOS-3576
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used to easily name imported libraries.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt f4feaf90b4750338cfa2127524a5e6556a86d063 
> 
> 
> Diff: https://reviews.apache.org/r/61285/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 61291: Imported `glog` library.

[Joseph Wu]

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


Ship it!




LGTM.  Going to add some whitespace around some comments though.

- Joseph Wu


On Aug. 11, 2017, 4:21 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61291/
> ---
> 
> (Updated Aug. 11, 2017, 4:21 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/61291
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt f4feaf90b4750338cfa2127524a5e6556a86d063 
> 
> 
> Diff: https://reviews.apache.org/r/61291/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 61690: Added `--zk_session_timeout` flag for agent.

[Ilya Pronin]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Currently `ZooKeeperMasterDetector` in agent uses default session
timeout and there's no way to configure it. This patch introduces
`--zk_session_timeout` flag for agent similar to the one in master.


Diffs
-

  src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
  src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 
  src/slave/main.cpp a4a8ced6825ece1b25003824d987ff83fb799ad7 


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


Testing
---

Ran `make check`.

Manually verified that ZK session negotiates 20 secs timeout instead of 10 secs 
when the agent is started with `--zk_session_timeout=20secs`.
```
2017-08-15 17:50:32,065:7763(0x7051c000):ZOO_INFO@check_events@1728: 
initiated connection to server [192.168.99.100:2181]
2017-08-15 17:50:32,069:7763(0x7051c000):ZOO_INFO@check_events@1775: 
session establishment complete on server [192.168.99.100:2181], 
sessionId=0x15de6c7f8920001, negotiated timeout=2
```


Thanks,

Ilya Pronin

Review Request 61689: Removed duplicate using directive.

[Ilya Pronin]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Removed duplicate using directive.


Diffs
-

  src/slave/main.cpp a4a8ced6825ece1b25003824d987ff83fb799ad7 


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


Testing
---

`make check`


Thanks,

Ilya Pronin

Re: Review Request 60622: Add new stout function: path::uri (convert filename to valid URI).

[Joseph Wu]

> On Aug. 15, 2017, 11:04 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 68 (patched)
> > 
> >
> > I don't think I would use `bool addprefix` for this logic. A URI by 
> > definition is _required_ to have the scheme. In fact, as far as I can tell 
> > from the RFC, the scheme and the host are the only required parts of a URI:
> > 
> >  scheme:[//[user[:password]@]host[:port]][/path][?query][#fragment]
> > 
> > So using this with `addprefix = false` would _not_ return a URI, and 
> > then the function no longer makes sense.
> > 
> > What use case drove the additoin of `bool addprefix`, can we solve it 
> > in another way?

Paths and URIs are fundamentally different things.  So none of this 
backslash->slash replacement should be happening in the `Path` class.

(Consider how Python has `os.path.join` and `urlparse.urljoin`.)

I have some (incomplete) patches that add URI parsing to stout (See: 
https://reviews.apache.org/r/46588/diff/4#3 ).  We basically need a URI class 
(without parsing for now) with some of the same helpers in the `path.hpp` file; 
especially something akin to `uri::join`.


- Joseph


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


On July 3, 2017, 12:30 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60622/
> ---
> 
> (Updated July 3, 2017, 12:30 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new stout function: path::uri (convert filename to valid URI).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 
>   3rdparty/stout/tests/path_tests.cpp 
> f8c14d5aefe0b49adb778da784143a328c96183d 
> 
> 
> Diff: https://reviews.apache.org/r/60622/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60620: Modify os::write to write binary files on Windows.

[Joseph Wu]

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


Ship it!




I'll fix this up before committing the chain.


3rdparty/stout/include/stout/os/write.hpp
Lines 70 (patched)


Should be an `#ifdef`.



3rdparty/stout/include/stout/os/write.hpp
Lines 71 (patched)


I'm going to drop a `NOTE:` comment here to explain the extra option.


- Joseph Wu


On July 5, 2017, 10:43 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60620/
> ---
> 
> (Updated July 5, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modify os::write to write binary files on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/write.hpp 
> beb5bd83b52565a75e34d32b5bb17951bc799578 
> 
> 
> Diff: https://reviews.apache.org/r/60620/diff/2/
> 
> 
> Testing
> ---
> 
> Built successfully on both Linux (with autotools and cmake) and Windows (with 
> cmake).
> 
> Ran stout-tests and mesos-tests successfully on both Windows and Linux.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 61189: Added authorization for V1 events.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61189]

Failed command: python support/apply-reviews.py -n -r 61189

Error:
error: patch failed: src/master/http.cpp:826
error: src/master/http.cpp: patch does not apply
error: patch failed: src/tests/api_tests.cpp:1921
error: src/tests/api_tests.cpp: patch does not apply

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/235/console

- Mesos Reviewbot Windows


On July 27, 2017, 6:25 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> ---
> 
> (Updated July 27, 2017, 6:25 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization filtering for V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
>   src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
>   src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32 
>   src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f 
> 
> 
> Diff: https://reviews.apache.org/r/61189/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> GLOG_v=2 ./bin/mesos-tests.sh 
> --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" 
> --verbose --gtest_repeat=100 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61189: Added authorization for V1 events.

[Alexander Rojas]

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



Can you also update the summary to mention that it only affects the `SUBSCRIBE` 
event.

- Alexander Rojas


On July 27, 2017, 8:25 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> ---
> 
> (Updated July 27, 2017, 8:25 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization filtering for V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
>   src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
>   src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32 
>   src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f 
> 
> 
> Diff: https://reviews.apache.org/r/61189/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> GLOG_v=2 ./bin/mesos-tests.sh 
> --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" 
> --verbose --gtest_repeat=100 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 61682: Fixed usage of 'ATOMIC_FLAG_INIT'.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61682]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 16, 2017, 4:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61682/
> ---
> 
> (Updated Aug. 16, 2017, 4:35 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The C++ standard guarantees that the macro 'ATOMIC_FLAG_INIT' is
> usable for initialization of atomic variables; its use in other
> context is unspecified.
> 
> libcxx defines 'ATOMIC_FLAG_INIT' as '{false}' which when used in an
> initializer list yields a clang warning,
> 
> warning: braces around scalar initializer [-Wbraced-scalar-init]
> lock(ATOMIC_FLAG_INIT)
> 
> This patch prevents such a warning by initializing 'atomic_flag'
> member variables in the class declaration instead of in an initializer
> list.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/grpc.hpp 
> 91c1cd9d6e3610e9978fae083fa75d781defa175 
>   3rdparty/libprocess/src/grpc.cpp 3ba5bc5415d55d7be044cfc0f4e240358cb3f4e8 
> 
> 
> Diff: https://reviews.apache.org/r/61682/diff/1/
> 
> 
> Testing
> ---
> 
> With clang-6.0.0: `./configure --enable-grpc && make -C 3rdparty/libprocess 
> tests` emits no compiler warning.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 61682: Fixed usage of 'ATOMIC_FLAG_INIT'.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Repository: mesos


Description
---

The C++ standard guarantees that the macro 'ATOMIC_FLAG_INIT' is
usable for initialization of atomic variables; its use in other
context is unspecified.

libcxx defines 'ATOMIC_FLAG_INIT' as '{false}' which when used in an
initializer list yields a clang warning,

warning: braces around scalar initializer [-Wbraced-scalar-init]
lock(ATOMIC_FLAG_INIT)

This patch prevents such a warning by initializing 'atomic_flag'
member variables in the class declaration instead of in an initializer
list.


Diffs
-

  3rdparty/libprocess/include/process/grpc.hpp 
91c1cd9d6e3610e9978fae083fa75d781defa175 
  3rdparty/libprocess/src/grpc.cpp 3ba5bc5415d55d7be044cfc0f4e240358cb3f4e8 


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


Testing
---

With clang-6.0.0: `./configure --enable-grpc && make -C 3rdparty/libprocess 
tests` emits no compiler warning.


Thanks,

Benjamin Bannier