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

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57624]

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

- Mesos Reviewbot


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

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

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

An unit test is augmented to check both injectAllocationInfo and
stripAllocationInfo.


Diffs
-

  src/tests/protobuf_utils_tests.cpp 2c32d56e41d12a26824aaf22ef78f8c0048acc00 


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


Testing
---

make check


Thanks,

Jay Guo

Review Request 57630: Renamed `adjustOfferOperation` to `injectAllocationInfo`.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Method name `protobuf::adjustOfferOperation` is renamed to
`protobuf::injectAllocationInfo`.


Diffs
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/master/allocator/mesos/hierarchical.cpp 
37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/hierarchical_allocator_tests.cpp 
dce619ec49db480685deb1bf8f7faeebe02e25b5 
  src/tests/protobuf_utils_tests.cpp 2c32d56e41d12a26824aaf22ef78f8c0048acc00 


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


Testing
---


Thanks,

Jay Guo

Re: Review Request 57340: Remove adjustment code within Resources::apply.

[Jay Guo]

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

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


Review request for mesos and Benjamin Mahler.


Changes
---

rebase


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


Repository: mesos


Description
---

Remove adjustment code within Resources::apply.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/allocator/mesos/hierarchical.cpp 
37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 


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

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


Testing
---

WIP


Thanks,

Jay Guo

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

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


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

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

[Benjamin Mahler]

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

Review request for mesos, haosdent huang and Joseph Wu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/webui/master/static/js/controllers.js 
4e1d07eb6155301c7d7d2b3e030c38156e8210ad 


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


Testing
---


Thanks,

Benjamin Mahler

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

[Benjamin Mahler]

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

(Updated March 15, 2017, 2:05 a.m.)


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


Changes
---

Fixed an issue where the roles tab is not shown as active (this was due to 
following the approach used in the maintenance tab, where it's also broken).


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01 
  src/webui/master/static/index.html 7811ecb2c8c4fc5d14c4d5ca690b611290b07c83 
  src/webui/master/static/js/app.js 73043a8521d2931b639ece11bf3f2b8bccba83d6 
  src/webui/master/static/js/controllers.js 
4e1d07eb6155301c7d7d2b3e030c38156e8210ad 
  src/webui/master/static/roles.html PRE-CREATION 


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

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


Testing
---

manual testing.


File Attachments


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


Thanks,

Benjamin Mahler

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

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [57427, 57426, 57425]

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

Error:
2017-03-15 01:45:35 URL:https://reviews.apache.org/r/57425/diff/raw/ 
[9700/9700] -> "57425.patch" [1]
error: patch failed: src/slave/containerizer/containerizer.hpp:151
error: src/slave/containerizer/containerizer.hpp: patch does not apply
error: patch failed: src/slave/containerizer/mesos/containerizer.hpp:114
error: src/slave/containerizer/mesos/containerizer.hpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/17437/console

- Mesos Reviewbot


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

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

[Jiang Yan Xu]

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

(Updated March 14, 2017, 6:09 p.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, 
and Vinod Kone.


Changes
---

Comments.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs (updated)
-

  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


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

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


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

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

[Jiang Yan Xu]

> On March 14, 2017, 5:08 p.m., Anindya Sinha wrote:
> > src/master/master.hpp
> > Lines 686 (patched)
> > 
> >
> > nit: s/authorizeSlave/authorizeAgent?

This is for the sake of consistency.

The master already/still has:

```
Master::addSlave
Master::removeSlave
...
```

I think we are maintaining consistency within old files and will later do a 
sweeping change. This is of course sometimes less straightfoward: e.g., you 
have an internal method that directly corresponds to a new API which already 
uses the new terminology, etc.


> On March 14, 2017, 5:08 p.m., Anindya Sinha wrote:
> > src/master/master.cpp
> > Line 5372 (original), 5446 (patched)
> > 
> >
> > nit: Is there a specific reason for the change here (and in 
> > reregisterSlave) from `from` to `pid`?

I am maintaining the pattern in the current `_registerSlave` and 
`_reregisterSlave`: 
https://github.com/apache/mesos/blob/5abaeec1ca53642bad329fce89fb3a03fbebc079/src/master/master.cpp#L5454

The way to look at it is:

`registerSlave` is a message handler: the `from` here is injected by libprocess.

In internal methods like `_registerSlave`, the meaning is changed: with

```
  void _registerSlave(
  const SlaveInfo& slaveInfo,
  const process::UPID& pid,
  const Option& principal,
  const std::vector& checkpointedResources,
  const std::string& version,
  const std::vector& agentCapabilities,
  const process::Future& authorized);
```

I am saying this method takes a `slaveInfo`, its associated pid is `pid`, its 
principal is `principal`, ...

So `from` is not as needed / as clear anymore.


- Jiang Yan


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


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

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

[Jiang Yan Xu]

> On March 14, 2017, 4:05 p.m., Anindya Sinha wrote:
> > include/mesos/authorizer/acls.proto
> > Line 440 (original), 450 (patched)
> > 
> >
> > Not your change, but maybe move that down so the ids are in ascending 
> > order to avoid issues in future?

Yeah this not being caught by protoc was due to my rebasing. 

In general ordering is not an issue so I am keep the current order.


> On March 14, 2017, 4:05 p.m., Anindya Sinha wrote:
> > include/mesos/authorizer/acls.proto
> > Lines 459 (patched)
> > 
> >
> > `remove_nested_containers` uses 33, so `register_agents` should be 34.

Yeah this is my bad rebasing.


- Jiang Yan


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


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

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

[Jiang Yan Xu]

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

(Updated March 14, 2017, 5:40 p.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, and Greg Mann.


Changes
---

Comments.


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


Repository: mesos


Description
---

Added and implemented RegisterAgent ACL.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto e75e1879435f1c2bce47a86e9feebf9d051e969b 
  include/mesos/authorizer/authorizer.proto 
736f76d552956f2351ffd40fc51d088dff83f8c8 
  src/authorizer/local/authorizer.cpp be8037299601427e5d5e79f58f77eea3f89579d0 
  src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 


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

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

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

[Anindya Sinha]

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




include/mesos/authorizer/acls.proto
Line 459 (original), 459 (patched)


Oh, I see this is fixed here. I think it is better to move this change to 
https://reviews.apache.org/r/57534



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


s/wehther/whether.



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


nit: s/authorizeSlave/authorizeAgent?



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


Similar comment as in header file. s/authorizeSlave/authorizeAgent?



src/master/master.cpp
Line 5372 (original), 5446 (patched)


nit: Is there a specific reason for the change here (and in 
reregisterSlave) from `from` to `pid`?


- Anindya Sinha


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

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

[Benjamin Mahler]

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




src/webui/master/static/roles.html
Lines 15 (patched)


Remove "data-sort" here.


- Benjamin Mahler


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

Re: Review Request 55887: Check task user before allowing a task to be launched on the agent.

[Jiang Yan Xu]

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


Ship it!




Committing soon with the following minor edits.


src/slave/slave.cpp
Lines 1997-1999 (patched)


The original location of this comment 

```
  // NOTE: Ideally we would perform the following check here:
  //
  //   if (framework->executors.empty() &&
  //   framework->pending.empty()) {
  // removeFramework(framework);
  //   }
  //
  // However, we need 'framework' to stay valid for the rest of
  // this function. As such, we perform the check before each of
  // the 'return' statements below.
```

should be here, where the tasks are removed not because of errors but 
because they are moved elsewhere.

Let me move it over and we can look into futher tidying things up.



src/slave/slave.cpp
Lines 2052-2055 (patched)


4-space indentation is usually for method arguments, use the following for 
continuing if conditions.

```
} else if (executorInfo.has_command() &&
   executorInfo.command().has_user()) {
  user = executorInfo.command().user();
}
```


- Jiang Yan Xu


On March 12, 2017, 12:05 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55887/
> ---
> 
> (Updated March 12, 2017, 12:05 a.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Alexander Rojas, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-6953
> https://issues.apache.org/jira/browse/MESOS-6953
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for action `run_tasks` on the agent's flag `acl`. Based on
> the ACL configured for `run_tasks`, a task to be launched on the agent
> can be (dis)allowed to launch on the agent.
> If a task or task group cannot be launched due to failed authorization,
> a `TASK_ERROR` Status Update shall be sent with a reason code of
> `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as
> applicable.
> Note that in case of a task group, all tasks fail if any of the tasks
> within the task group encounter the authorization error.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 978edd6309dfbbde1058f9c44d5fac7083ff95fb 
>   src/slave/slave.cpp 2308d5bf1fef5e1a6458a3bb742a16935a127929 
> 
> 
> Diff: https://reviews.apache.org/r/55887/diff/11/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57340: Remove adjustment code within Resources::apply.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57340]

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

- Mesos Reviewbot


On March 14, 2017, 5:39 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57340/
> ---
> 
> (Updated March 14, 2017, 5:39 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7048
> https://issues.apache.org/jira/browse/MESOS-7048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove adjustment code within Resources::apply.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
>   src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
>   src/master/allocator/mesos/hierarchical.cpp 
> 37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
>   src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 
> 
> 
> Diff: https://reviews.apache.org/r/57340/diff/2/
> 
> 
> Testing
> ---
> 
> WIP
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

[Avinash sridharan]

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1093-1097 (patched)
> > 
> >
> > Instead of that, can we use `__MESOS_TEST__2`?
> 
> Avinash sridharan wrote:
> Hi Jie, 
> Sorry for the delay here. We will need to change the `network/cni` 
> isolator for this right? Didn't really see a reason for this since replacing 
> the existing CNI config would just work the same. Will keep the logic in the 
> `network/cni` isolator simpler?
> 
> Jie Yu wrote:
> I thought we already do 'contains' check in CNI isolator?
> 
> Avinash sridharan wrote:
> 
> https://github.com/apache/mesos/blob/7c3c0b03a8371922f6cc811ad74d85c97eefb494/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L693
> 
> I think you are referring to using `strings::contains`. No unfortunately 
> haven't added that to the `network/cni` isolator.
> 
> Jie Yu wrote:
> Can we do that. I remember you already have the patch?
> 
> Avinash sridharan wrote:
> We were planning to do that for the tests for dynamically 
> `add/delete/modify` CNI configuration, but as in this case it turned out we 
> didn't need to so ended up not implementing it. Let me see if I can do it as 
> part of this ticket.

Created a separate patch for the changes to the `network/cni` isolator.


- Avinash


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


On March 14, 2017, 11:13 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53264/
> ---
> 
> (Updated March 14, 2017, 11:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6022
> https://issues.apache.org/jira/browse/MESOS-6022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for CNI port-mapper plugin.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb893d3ef005a9cc60c40768fa669b27c4863020 
> 
> 
> Diff: https://reviews.apache.org/r/53264/diff/6/
> 
> 
> Testing
> ---
> 
> sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

[Avinash sridharan]

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

(Updated March 14, 2017, 11:13 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Refactored the code to use separate test network for port-mapper tests.


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


Repository: mesos


Description
---

Added test for CNI port-mapper plugin.


Diffs (updated)
-

  src/tests/containerizer/cni_isolator_tests.cpp 
cb893d3ef005a9cc60c40768fa669b27c4863020 


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

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


Testing (updated)
---

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

Review Request 57628: Added a utility function to get back non-loopback address on the host.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added a utility function to get back non-loopback address on the host.


Diffs
-

  src/tests/utils.hpp cdbbc1c611bbcf2d6548f6b65fb08c6a4831b7a8 
  src/tests/utils.cpp 0a9e5a867a46795f01fcf7030f50581b5ef1341f 


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


Testing
---

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

Re: Review Request 57627: Modified CNI isolator to treat "__MESOST_TEST__*" as a test network.

[Avinash sridharan]

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

(Updated March 14, 2017, 11:05 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Currently the CNI isolator identifies the "__MESOS_TEST__" as a test
network. However, there are cases where we might need multiple test
networks to test against. To support this use case we now do a
substring match on the network name and treat any network with name of
the form "*__MESOS_TEST__*" as a test network.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 


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


Testing
---

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

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

[Anindya Sinha]

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




include/mesos/authorizer/acls.proto
Line 440 (original), 450 (patched)


Not your change, but maybe move that down so the ids are in ascending order 
to avoid issues in future?



include/mesos/authorizer/acls.proto
Lines 459 (patched)


`remove_nested_containers` uses 33, so `register_agents` should be 34.


- Anindya Sinha


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

Review Request 57627: Modified CNI isolator to treat "__MESOST_TEST__*" as a test network.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Currently the CNI isolator identifies the "__MESOS_TEST__" as a test
network. However, there are cases where we might need multiple test
networks to test against. To support this use case we now do a
substring match on the network name and treat any network with name of
the form "*__MESOS_TEST__*" as a test network.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 


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


Testing
---

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

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

[Benjamin Mahler]

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

Review request for mesos, haosdent huang and Joseph Wu.


Repository: mesos


Description
---

This follows the established approach used in the other controllers.


Diffs
-

  src/webui/master/static/js/controllers.js 
4e1d07eb6155301c7d7d2b3e030c38156e8210ad 


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


Testing
---


Thanks,

Benjamin Mahler

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

[Benjamin Mahler]

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

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


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


Repository: mesos


Description
---

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


Diffs
-

  src/webui/master/static/index.html 7811ecb2c8c4fc5d14c4d5ca690b611290b07c83 
  src/webui/master/static/js/app.js 73043a8521d2931b639ece11bf3f2b8bccba83d6 
  src/webui/master/static/js/controllers.js 
4e1d07eb6155301c7d7d2b3e030c38156e8210ad 
  src/webui/master/static/roles.html PRE-CREATION 


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


Testing
---

manual testing.


File Attachments


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


Thanks,

Benjamin Mahler

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

[Anand Mazumdar]

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

Review request for mesos and Zhitao Li.


Repository: mesos


Description
---

The `AGENT_REMOVED` event is not sent when the agent does not
reregister within the timeout. In the future, we would introduce
a new event that is sent when an agent is marked as unreachable.


Diffs
-

  include/mesos/master/master.proto 0d8d58983c1ce5553845b0a01df33dca3ac90433 
  include/mesos/v1/master/master.proto da6f1104fabfe6c09a4cb4ae09ed91bc70827a71 


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


Testing
---

Not a functional change.


Thanks,

Anand Mazumdar

Re: Review Request 57167: Updated quota handler logic for hierarchical roles.

[Michael Park]

> On March 8, 2017, 10:38 a.m., Michael Park wrote:
> > src/master/quota_handler.cpp
> > Lines 78 (patched)
> > 
> >
> > The general pattern for `validate` I think is to return an 
> > `Option`. In this case, we could more accurately report what/where 
> > the invalid relationship is within the tree.
> 
> Neil Conway wrote:
> `Option` seemed like overkill to me, and doesn't compose as nicely 
> when checking the validity of children. I renamed the member function to 
> `isValid()` -- how does that sound?

> Option seemed like overkill to me

Okay
> and doesn't compose as nicely when checking the validity of children.

Hm...? I don't get it. Doesn't it just go from
```cpp
bool isValid() const
{
  foreachvalue (const unique_ptr& child, root->children) {
if (!child->isValid()) {
  return false;
}
  }
  
  return true;
}
```

to...

```cpp
Option validate() const
{
  foreachvalue (const unique_ptr& child, root->children) {
Option validate = child->validate();
if (validate.isSome()) {
  return validate;
}
  }
  
  return None();
}
```

and,

```cpp
bool isValid() const
{
  foreachvalue (const unique_ptr& child, children) {
if (!child->isValid()) {
  return false;
}
  }

  Resources childResources;
  foreachvalue (const unique_ptr& child, children) {
childResources += child->quota.info.guarantee();
  }

  Resources selfResources = quota.info.guarantee();

  return selfResources.contains(childResources);
}
```

to...

```cpp
Option validate() const
{
  foreachvalue (const unique_ptr& child, children) {
Option validate = child->validate();
if (validate.isSome()) {
  return validate;
}
  }

  Resources childResources;
  foreachvalue (const unique_ptr& child, children) {
childResources += child->quota.info.guarantee();
  }

  Resources selfResources = quota.info.guarantee();

  if (!selfResources.contains(childResources)) {
return Error("Detected invalid quota tree. Parent role " + name +
 " with quota: " + selfResources + " does not contain"
 "the sum of its children's resources: " + childResources);
  }

  return None();
}
```

It's fine if you think `validate` is overkill, but I'm not seeing what doesn't 
work out nicely.
To me it just seems like it's not any more complicated, and produces a better 
error message for the user.


- Michael


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


On March 10, 2017, 2:03 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> ---
> 
> (Updated March 10, 2017, 2:03 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
> 
> When creating and removing quota, we must ensure that this invariant is
> not violated.
> 
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 CPUs, role "x" has a quota guarantee of 80 CPUs, and
> role "x/y" has a quota guarantee of 40 CPUs, this does NOT violate the
> cluster capacity heuristic.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67 
>   src/tests/hierarchical_allocator_tests.cpp 
> dce619ec49db480685deb1bf8f7faeebe02e25b5 
>   src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837 
> 
> 
> Diff: https://reviews.apache.org/r/57167/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Greg Mann]

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

(Updated March 14, 2017, 9:36 p.m.)


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


Changes
---

Added new cpp file to the cmake build.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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


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

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 57608: Augmented master api test to check `GetRoles` is included.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55252, 55253, 57592, 57605, 57606, 57607, 57608]

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

- Mesos Reviewbot


On March 14, 2017, 5:11 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57608/
> ---
> 
> (Updated March 14, 2017, 5:11 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6855
> https://issues.apache.org/jira/browse/MESOS-6855
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checks that `GetRoles` is included in `GetState` API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 29ae1bcf660fb0e03af1d2192484c9ec739f3ef6 
> 
> 
> Diff: https://reviews.apache.org/r/57608/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57491: Windows: Update libprocess CMake setup for Glog patch.

[Joseph Wu]

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


Ship it!




I can fix those above nits before committing.

- Joseph Wu


On March 9, 2017, 5:59 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57491/
> ---
> 
> (Updated March 9, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6815
> https://issues.apache.org/jira/browse/MESOS-6815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The additional Windows library `Dbghelp` must be linked for the
> patches enabling stack traces.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> e9eabd08d327f4c714199f7bde91b9adf644a678 
> 
> 
> Diff: https://reviews.apache.org/r/57491/diff/1/
> 
> 
> Testing
> ---
> 
> Run CTest. Add purposefully failing test, observe stack trace.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57490: Windows: Add Glog patch for enabling stack trace.

[Joseph Wu]

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


Ship it!




LGTM!

Verified a couple of artificially added CHECK failures in stout, libprocess, 
and Mesos all stacktraced.

Followup for stacktraces on segfaults: 
https://issues.apache.org/jira/browse/MESOS-7245

- Joseph Wu


On March 9, 2017, 5:59 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57490/
> ---
> 
> (Updated March 9, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6815
> https://issues.apache.org/jira/browse/MESOS-6815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This relies on an updated `glog-da816ea70.tar.gz` tarball
> in the Mesos 3rdparty repository, and a patch generated between
> it and the `windows-stacktrace` branch (and pull request) made
> to Glog.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 5fbe178c3d4335bb30c5b955fbde8861648c0496 
>   3rdparty/glog-da816ea70.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57490/diff/1/
> 
> 
> Testing
> ---
> 
> Run CTest.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56474, 56476, 56475]

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

- Mesos Reviewbot


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

Review Request 57535: Applied RegisterAgent ACL to the master.

[Jiang Yan Xu]

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

Review request for mesos, Adam B, Anindya Sinha, Greg Mann, and Vinod Kone.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs
-

  include/mesos/authorizer/acls.proto e75e1879435f1c2bce47a86e9feebf9d051e969b 
  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


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


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

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

[Andrew Schwartzmeyer]

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




src/tests/check_tests.cpp
Lines 74 (patched)


As the resident PowerShell "expert" (heh!), I tested this (and the second 
one) and they do what you're expecting.

Just a couple notes:

You might be wanting to use `Test-Path "somepath"` instead, which returns a 
boolean for existence of that path, so you can do `if (Test-Path "somepath")` 
instead of the gunk around checking `$?`. Also, semicolons are unnecssary in 
PowerShell except to separate multiple commands on one line. If you want it to 
be _perfect_ we try to prefer the `Camel-Case` of cmdlet names, but it's 
whatever.


- Andrew Schwartzmeyer


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

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

[Jiang Yan Xu]

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

(Updated March 14, 2017, 11:17 a.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added and implemented RegisterAgent ACL.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto e75e1879435f1c2bce47a86e9feebf9d051e969b 
  include/mesos/authorizer/authorizer.proto 
736f76d552956f2351ffd40fc51d088dff83f8c8 
  src/authorizer/local/authorizer.cpp be8037299601427e5d5e79f58f77eea3f89579d0 
  src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 


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

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 57340: Remove adjustment code within Resources::apply.

[Jay Guo]

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

(Updated March 15, 2017, 1:39 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Address bmahler's comments.


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


Repository: mesos


Description
---

Remove adjustment code within Resources::apply.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/allocator/mesos/hierarchical.cpp 
37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 


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

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


Testing
---

WIP


Thanks,

Jay Guo

Re: Review Request 53074: Updated pylint to rebuild 'virtualenv' when necessary.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 12, 2017, 4:35 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53074/
> ---
> 
> (Updated March 12, 2017, 4:35 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6430
> https://issues.apache.org/jira/browse/MESOS-6430
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated pylint to rebuild 'virtualenv' when necessary.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 3a5bec81950ccf8083cbd73577c49b2fb3b910f8 
> 
> 
> Diff: https://reviews.apache.org/r/53074/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57340: Remove adjustment code within Resources::apply.

[Jay Guo]

> On March 14, 2017, 7:03 a.m., Benjamin Mahler wrote:
> > Looks good, the changes to resources.cpp were done how? Are they a direct 
> > reversion to the old code?

Yes, I did a `git revert` and apply changes based on that.


> On March 14, 2017, 7:03 a.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.hpp
> > Lines 126-128 (original), 126-128 (patched)
> > 
> >
> > I like the naming you used for the strip function, mind also clarifying 
> > the naming of this one in a separate patch?
> > 
> > ```
> > void injectAllocationInfo(
> > Offer::Operation* operation,
> > const Resource::AllocationInfo& allocationInfo);
> > ```

Will do.


- Jay


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


On March 15, 2017, 1:39 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57340/
> ---
> 
> (Updated March 15, 2017, 1:39 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7048
> https://issues.apache.org/jira/browse/MESOS-7048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove adjustment code within Resources::apply.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
>   src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
>   src/master/allocator/mesos/hierarchical.cpp 
> 37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
>   src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 
> 
> 
> Diff: https://reviews.apache.org/r/57340/diff/2/
> 
> 
> Testing
> ---
> 
> WIP
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57491: Windows: Update libprocess CMake setup for Glog patch.

[Joseph Wu]

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




3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake
Lines 34-36 (original), 34-36 (patched)


One more nit:
We should update this comment to reference `da816ea70` since we're now 
using a glog version past the latest release of glog.


- Joseph Wu


On March 9, 2017, 5:59 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57491/
> ---
> 
> (Updated March 9, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6815
> https://issues.apache.org/jira/browse/MESOS-6815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The additional Windows library `Dbghelp` must be linked for the
> patches enabling stack traces.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> e9eabd08d327f4c714199f7bde91b9adf644a678 
> 
> 
> Diff: https://reviews.apache.org/r/57491/diff/1/
> 
> 
> Testing
> ---
> 
> Run CTest. Add purposefully failing test, observe stack trace.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 57607: Added `GetRoles` to `GetState` API.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added `GetRoles` to `GetState` API.


Diffs
-

  src/master/http.cpp 862b68ff2e8846d6fb968556a6612ec7dab2a61a 
  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 


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


Testing
---


Thanks,

Jay Guo

Review Request 57608: Augmented master api test to check `GetRoles` is included.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Checks that `GetRoles` is included in `GetState` API.


Diffs
-

  src/tests/api_tests.cpp 29ae1bcf660fb0e03af1d2192484c9ec739f3ef6 


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


Testing
---


Thanks,

Jay Guo

Review Request 57606: Extracted some logic from `getRoles` into `_getRoles`.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Some logic from `getRoles` is extracted into `_getRoles` so that it
could be reused by `_getState`. This is a step toward adding roles
to `GetState` V1 API.


Diffs
-

  src/master/http.cpp 862b68ff2e8846d6fb968556a6612ec7dab2a61a 
  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 


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


Testing
---


Thanks,

Jay Guo

Review Request 57605: Added `GetRoles` to `GetState` in master.proto.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added `GetRoles` to `GetState` in master.proto.


Diffs
-

  include/mesos/master/master.proto 0d8d58983c1ce5553845b0a01df33dca3ac90433 
  include/mesos/v1/master/master.proto da6f1104fabfe6c09a4cb4ae09ed91bc70827a71 


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


Testing
---


Thanks,

Jay Guo

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

[Greg Mann]

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

(Updated March 14, 2017, 5:07 p.m.)


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


Changes
---

Rebase.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/tests/http_authentication_tests.cpp 
0eeed9d19881cf3fa5fec7fb7fedc1e92784f58b 


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

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


Testing
---

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


Thanks,

Greg Mann

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

[Greg Mann]

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

(Updated March 14, 2017, 5:07 p.m.)


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


Changes
---

Rebase.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf 


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

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


Testing
---

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


Thanks,

Greg Mann

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

[Greg Mann]

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

(Updated March 14, 2017, 5:07 p.m.)


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


Changes
---

Rebase.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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


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

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


Testing
---

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


Thanks,

Greg Mann

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

[Alexander Rukletsov]

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

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


Repository: mesos


Description
---

See summary.


Diffs
-

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


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


Testing
---

make check


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:09 p.m.)


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


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
  src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

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


Repository: mesos


Description
---

See summary.


Diffs
-

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


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


Testing
---

make check


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:09 p.m.)


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


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:08 p.m.)


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

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


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


Repository: mesos


Description
---

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


Diffs
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:07 p.m.)


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


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:07 p.m.)


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


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


Repository: mesos


Description
---

Also validate internal invariants when health is updated.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:06 p.m.)


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

> On March 1, 2017, 8:23 p.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Line 424 (original), 440 (patched)
> > 
> >
> > why are we sending WEXITSTATUS and not exit code?
> 
> Alexander Rukletsov wrote:
> Because what we get from `Subprocess` is actually `pid_t`. It is unclear 
> what the scheduler will do with the pid.
> 
> Vinod Kone wrote:
> AFAIK the status code returned by `waitpid()` has strictly more 
> information (whether a process has exited, signalled or stopped). So I can 
> imagine scheduler can make better decisions with extra information? It's a 
> bit weird that we are not sending check status if a command exits because of 
> signaling or stopping?
> 
> Also, AFAIK we send status code in the executor terminated event as well? 
> So it would be nice if we can be consistent unless there is a strong reason 
> to differ.
> 
> Alexander Rukletsov wrote:
> Correcting my answer above. What we get from `Subprocess` is 
> `status_value` (see man on `waitpid`), which embeds exit code and has extra 
> information. I'm not sure whether a scheduler is interested in fine grained 
> termination information for a check.
> 
> I would argue it may be more surprising for 3rdparty tools to see Posix's 
> internal `status_value` instead of a plain exit code. Moreover, IIUC to 
> extract exit code from `status_value`, a scheduler should know whether the 
> message is coming from Posix or Windows agent and hence either apply extra 
> interpretation or use the value as is.
> 
> However, I do confirm that executor termination event contains 
> `status_value`.

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


- Alexander


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


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

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:05 p.m.)


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


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

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


Repository: mesos


Description
---

See summary.


Diffs
-

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


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


Testing
---

make check


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:05 p.m.)


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


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

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


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

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


Testing
---

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


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:05 p.m.)


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

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


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

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated March 14, 2017, 2:05 p.m.)


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

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


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

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 57593: Sorted enum values in `authorizer.proto` by numerical order.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 14, 2017, 11:20 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57593/
> ---
> 
> (Updated March 14, 2017, 11:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Aliasing is allowed in `org.apache.mesos.authorization.Actions`, so the
> values have to be kept in numerical order in order to prevent accidental
> aliasing.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 617bf6e0a3a39d8164635d481c181ef50148a095 
> 
> 
> Diff: https://reviews.apache.org/r/57593/diff/1/
> 
> 
> Testing
> ---
> 
> `make`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57593: Sorted enum values in `authorizer.proto` by numerical order.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On March 14, 2017, 11:20 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57593/
> ---
> 
> (Updated March 14, 2017, 11:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Aliasing is allowed in `org.apache.mesos.authorization.Actions`, so the
> values have to be kept in numerical order in order to prevent accidental
> aliasing.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 617bf6e0a3a39d8164635d481c181ef50148a095 
> 
> 
> Diff: https://reviews.apache.org/r/57593/diff/1/
> 
> 
> Testing
> ---
> 
> `make`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57386: Introduced changes to the authz protos needed for RemoveNestedContainer.

[Gastón Kleiman]

> On March 13, 2017, 9:15 p.m., Jiang Yan Xu wrote:
> > include/mesos/authorizer/authorizer.proto
> > Lines 174 (patched)
> > 
> >
> > The comment above says 
> > 
> > ```
> > // NOTE: Values in this enum should be kept in
> > // numerical order to prevent accidental aliasing.
> > ```
> > 
> > ?

Good catch! Fixed this in https://reviews.apache.org/r/57593/.


- Gastón


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


On March 10, 2017, 6:02 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57386/
> ---
> 
> (Updated March 10, 2017, 6:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Alexander Rojas, Jie 
> Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7120
> https://issues.apache.org/jira/browse/MESOS-7120
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced changes to the authz protos needed for RemoveNestedContainer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8389917d12f9a9a3c9b4281f48e23ade14c20528 
>   include/mesos/authorizer/authorizer.proto 
> fdc4817ce74c45d792fc47f064f7909a83b1657d 
> 
> 
> Diff: https://reviews.apache.org/r/57386/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 57593: Sorted enum values in `authorizer.proto` by numerical order.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov, Vinod Kone, and Jiang Yan Xu.


Repository: mesos


Description
---

Aliasing is allowed in `org.apache.mesos.authorization.Actions`, so the
values have to be kept in numerical order in order to prevent accidental
aliasing.


Diffs
-

  include/mesos/authorizer/authorizer.proto 
617bf6e0a3a39d8164635d481c181ef50148a095 


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


Testing
---

`make`


Thanks,

Gastón Kleiman

Re: Review Request 57560: Removed unnecessary curly braces wrapping case statements.

[Gastón Kleiman]

> On March 13, 2017, 8:28 p.m., Jiang Yan Xu wrote:
> > What's the criteria for deeming such braces necessary or not? Some switch 
> > cases in authorizer.cpp are left with braces. These cases are long of 
> > course, but is this the critera used and "how long/complex is too 
> > long/complex"?

The idea of the patch was to be consistent at least within the same file. I 
would also be fine with adding braces to all the cases.

Only the following two switch cases still have with braces:

https://github.com/apache/mesos/blob/fa7f4090a80b352dbc8bb9c5ee5053970e817826/src/authorizer/local/authorizer.cpp#L416-L446
https://github.com/apache/mesos/blob/fa7f4090a80b352dbc8bb9c5ee5053970e817826/src/authorizer/local/authorizer.cpp#L837-L866

I left the braces not because the cases are too long/complex, but because new 
variables are declared there, and the file doesn't compile without the braces:


```
../../src/authorizer/local/authorizer.cpp: In member function ‘virtual 
Try mesos::internal::LocalAuthorizerObjectApprover::approved(const 
Option&) const’:
../../src/authorizer/local/authorizer.cpp:446:29: error: jump to case label 
[-fpermissive]
 case authorization::VIEW_EXECUTOR:
 ^
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses 
initialization of ‘Option taskUser’
   Option taskUser = None();
  ^~~~
../../src/authorizer/local/authorizer.cpp:459:29: error: jump to case label 
[-fpermissive]
 case authorization::LAUNCH_NESTED_CONTAINER:
 ^~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses 
initialization of ‘Option taskUser’
   Option taskUser = None();
  ^~~~
../../src/authorizer/local/authorizer.cpp:460:29: error: jump to case label 
[-fpermissive]
 case authorization::LAUNCH_NESTED_CONTAINER_SESSION:
 ^~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses 
initialization of ‘Option taskUser’
   Option taskUser = None();
  ^~~~
../../src/authorizer/local/authorizer.cpp:482:29: error: jump to case label 
[-fpermissive]
 case authorization::VIEW_CONTAINER:
 ^~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses 
initialization of ‘Option taskUser’
   Option taskUser = None();
  ^~~~
../../src/authorizer/local/authorizer.cpp:496:29: error: jump to case label 
[-fpermissive]
 case authorization::SET_LOG_LEVEL:
 ^
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses 
initialization of ‘Option taskUser’
   Option taskUser = None();
  ^~~~
../../src/authorizer/local/authorizer.cpp:500:29: error: jump to case label 
[-fpermissive]
 case authorization::UNKNOWN:
 ^~~
../../src/authorizer/local/authorizer.cpp:422:26: note:   crosses 
initialization of ‘Option taskUser’
   Option taskUser = None();
  ^~~~
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value 
‘UNKNOWN’ not handled in switch [-Werror=switch]
   switch (action_) {
  ^
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value 
‘VIEW_EXECUTOR’ not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value 
‘LAUNCH_NESTED_CONTAINER’ not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value 
‘LAUNCH_NESTED_CONTAINER_SESSION’ not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value 
‘VIEW_CONTAINER’ not handled in switch [-Werror=switch]
../../src/authorizer/local/authorizer.cpp:223:14: error: enumeration value 
‘SET_LOG_LEVEL’ not handled in switch [-Werror=switch]
cc1plus: all warnings being treated as errors
```


- Gastón


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


On March 13, 2017, 6:19 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57560/
> ---
> 
> (Updated March 13, 2017, 6:19 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---

Review Request 57592: Augmented a master test to check `roles` in `/state` endpoint.

[Jay Guo]

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

Review request for mesos, Benjamin Mahler and Michael Park.


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


Repository: mesos


Description
---

Augmented a master test to check `roles` in `/state` endpoint.


Diffs
-

  src/tests/master_tests.cpp 5fdb9493c9aed31b90e03062544c1446eb200040 


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


Testing
---

make check

[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from MasterTest
[ RUN  ] MasterTest.StateEndpointRoles
[   OK ] MasterTest.StateEndpointRoles (111 ms)
[--] 1 test from MasterTest (112 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (143 ms total)
[  PASSED  ] 1 test.


Thanks,

Jay Guo

Re: Review Request 55253: Added `roles` to `/state` endpoint of master.

[Jay Guo]

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

(Updated March 14, 2017, 6:29 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Summary (updated)
-

Added `roles` to `/state` endpoint of master.


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


Repository: mesos


Description (updated)
---

`/state` endpoint of master is augmented to include `roles`, which
is identical to the response of `/roles`.


Diffs (updated)
-

  src/master/http.cpp 862b68ff2e8846d6fb968556a6612ec7dab2a61a 


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

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55252: Moved some logic from `Master::Http::_roles` into `filterRoles`.

[Jay Guo]

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

(Updated March 14, 2017, 6:29 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Summary (updated)
-

Moved some logic from `Master::Http::_roles` into `filterRoles`.


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


Repository: mesos


Description (updated)
---

Moved some logic in `Master::Http::_roles` into a new method
`filterRoles`, which could be reused by `Master::Http::state`.


Diffs (updated)
-

  src/master/http.cpp 862b68ff2e8846d6fb968556a6612ec7dab2a61a 
  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 


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

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


Testing
---


Thanks,

Jay Guo

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

[Alexander Rukletsov]

> On March 1, 2017, 8:23 p.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Line 424 (original), 440 (patched)
> > 
> >
> > why are we sending WEXITSTATUS and not exit code?
> 
> Alexander Rukletsov wrote:
> Because what we get from `Subprocess` is actually `pid_t`. It is unclear 
> what the scheduler will do with the pid.
> 
> Vinod Kone wrote:
> AFAIK the status code returned by `waitpid()` has strictly more 
> information (whether a process has exited, signalled or stopped). So I can 
> imagine scheduler can make better decisions with extra information? It's a 
> bit weird that we are not sending check status if a command exits because of 
> signaling or stopping?
> 
> Also, AFAIK we send status code in the executor terminated event as well? 
> So it would be nice if we can be consistent unless there is a strong reason 
> to differ.

Correcting my answer above. What we get from `Subprocess` is `status_value` 
(see man on `waitpid`), which embeds exit code and has extra information. I'm 
not sure whether a scheduler is interested in fine grained termination 
information for a check.

I would argue it may be more surprising for 3rdparty tools to see Posix's 
internal `status_value` instead of a plain exit code. Moreover, IIUC to extract 
exit code from `status_value`, a scheduler should know whether the message is 
coming from Posix or Windows agent and hence either apply extra interpretation 
or use the value as is.

However, I do confirm that executor termination event contains `status_value`.


- Alexander


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


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