Re: Review Request 60105: Clean rebooted slave's state if slaveInfo mismatches.

[Megha Sharma]

> On June 21, 2017, 4:39 p.m., Jiang Yan Xu wrote:
> > PTAL at these comments but I realize for this review it's probably easier 
> > if we sync on the high-level strategy first.

Rearranged the code as per our discussion.


- Megha


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


On June 22, 2017, 11:38 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> ---
> 
> (Updated June 22, 2017, 11:38 p.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60105/diff/6/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 56895
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 60105: Clean rebooted slave's state if slaveInfo mismatches.

[Megha Sharma]

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

(Updated June 22, 2017, 11:38 p.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.


Diffs (updated)
-

  src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 


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

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


Testing
---

make check done together with 60104 and 56895


Thanks,

Megha Sharma

Re: Review Request 60351: Updated the `operator<<` for repeated resources to use `JSON::protobuf`.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On June 22, 2017, 12:08 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60351/
> ---
> 
> (Updated June 22, 2017, 12:08 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `Resource` objects in a `RepeatedPtrField` may not be
> validated and/or not converted to the "post-reservation-refinement"
> format. Since `Resources` requires the resources to have been validated
> and converted, we cannot rely on the conversion to be meaningful.
> We opt to print the resources in their JSON representation instead.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 1d9170a9836799cf3764fbb37cd2e2aebd3e75b3 
>   src/v1/resources.cpp 58a00e933ad16481d3bdec6e273154ea93221830 
> 
> 
> Diff: https://reviews.apache.org/r/60351/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60352: Relaxed the resource format restriction for frameworks.

[Neil Conway]

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


Ship it!




Per offline discussion, add more on rationale to commit message.

- Neil Conway


On June 22, 2017, 12:32 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60352/
> ---
> 
> (Updated June 22, 2017, 12:32 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch relaxes the validation such that a framework with
> RESERVATION_REFINEMENT capability can send the old resources format as
> well. This is more of a temporary solution due to the complexity of
> the validation code, and we will revisit whether we want to restrict
> RESERVATION_REFINEMENT-capable frameworks to the new format. MESOS-7705.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp 129505948c0fad28743e290f24df55918c8d601c 
>   src/master/validation.cpp 4d0af26fedb6ef3039536cb42d2ac9112c9f4bdd 
> 
> 
> Diff: https://reviews.apache.org/r/60352/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60377: Added test for reservation refinements and old agents.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On June 22, 2017, 2:18 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60377/
> ---
> 
> (Updated June 22, 2017, 2:18 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the agent does not support reservation refinements, attempts to
> create a reservation refinement on the agent should be dropped by the
> master.
> 
> 
> Diffs
> -
> 
>   src/tests/upgrade_tests.cpp eef81018ab3fae2e91077d4314cdaba274111401 
> 
> 
> Diff: https://reviews.apache.org/r/60377/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`, ran ~5k iterations w/o error.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

[Neil Conway]

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


Fix it, then Ship it!





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


We use `EXPECT_EQ` in the other tests here.


- Neil Conway


On June 22, 2017, 10:22 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60283/
> ---
> 
> (Updated June 22, 2017, 10:22 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, it seemed like calling `convertResourceFormat` after
> operation validation seemed safe since the operation validation
> themselves performed `Resources::validate` within them.
> 
> However, the rest of the operation validation code relies on
> the fact that the resources have been validated, and uses
> functions such as `isDynamicallyReserved`. Since functions such as
> `isDynamicallyReserved` now requires "post-reservation-refinement"
> format, we must perform this conversion earlier.
> 
> In this patch, we use `upgradeResources` to perform resources
> validation __and__ convert the resources before going into the
> operation and task validation.
> 
> We really need a better plan for this going forward. MESOS-7702.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b0818e1e59ce9804cd9592aa2a7ec8f80ba5bddf 
>   src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
>   src/tests/resource_offers_tests.cpp 
> c2bbf834c1d46079af492887b9dd40e57f3f2ac7 
> 
> 
> Diff: https://reviews.apache.org/r/60283/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

[Michael Park]

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

(Updated June 22, 2017, 3:22 p.m.)


Review request for mesos and Neil Conway.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Initially, it seemed like calling `convertResourceFormat` after
operation validation seemed safe since the operation validation
themselves performed `Resources::validate` within them.

However, the rest of the operation validation code relies on
the fact that the resources have been validated, and uses
functions such as `isDynamicallyReserved`. Since functions such as
`isDynamicallyReserved` now requires "post-reservation-refinement"
format, we must perform this conversion earlier.

In this patch, we use `upgradeResources` to perform resources
validation __and__ convert the resources before going into the
operation and task validation.

We really need a better plan for this going forward. MESOS-7702.


Diffs (updated)
-

  src/master/master.cpp b0818e1e59ce9804cd9592aa2a7ec8f80ba5bddf 
  src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
  src/tests/resource_offers_tests.cpp c2bbf834c1d46079af492887b9dd40e57f3f2ac7 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60284: Removed unused `convertResourceFormat` for `Operation`.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On June 21, 2017, 7:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60284/
> ---
> 
> (Updated June 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused `convertResourceFormat` for `Operation`.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.hpp 4b033687fa7224ffa36523cb3ca366f4384fd4f8 
>   src/common/resources_utils.cpp 751194c5ff5c794c77be3bcf16892af39de6d4d9 
> 
> 
> Diff: https://reviews.apache.org/r/60284/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60255: Prevented reserve/create with refined reservation on non-capable agents.

[Neil Conway]

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


Ship it!




We can do this later, but would be nice to have unit tests for this -- similar 
to `CreateOperationValidationTest.AgentHierarchicalRoleCapability`

- Neil Conway


On June 20, 2017, 11:58 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60255/
> ---
> 
> (Updated June 20, 2017, 11:58 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7700
> https://issues.apache.org/jira/browse/MESOS-7700
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes it such that reserve / create operations involving
> resources with refined reservations are invalid if they are attempted
> on an agent without a RESERVATION_REFINEMENT capability. Such attempts
> from an operator endpoint will result in a `BadRequest` response.
> On the framework side, the operation will be dropped by the master.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 4d0af26fedb6ef3039536cb42d2ac9112c9f4bdd 
> 
> 
> Diff: https://reviews.apache.org/r/60255/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

[Neil Conway]

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


Fix it, then Ship it!





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


remove "after validation"



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


Per discussion, would be good to consider whether to validate/upgrade a 
task and its resources in a single operation.



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


`using` for `google::protobuf::RepeatedPtrField` ?



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


Can we add a comment here?

```
// If reservation refinement is enabled, inbound
// resources should already be in the "post-refinement" format and should 
not need to be upgraded.
```



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


Comment would be helpful here also.



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


Can we remove the `url` stuff?



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


Update or remove this comment.



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


"a RESERVE"



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


"an UNRESERVE"



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


"we test"


- Neil Conway


On June 21, 2017, 7:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60283/
> ---
> 
> (Updated June 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, it seemed like calling `convertResourceFormat` after
> operation validation seemed safe since the operation validation
> themselves performed `Resources::validate` within them.
> 
> However, the rest of the operation validation code relies on
> the fact that the resources have been validated, and uses
> functions such as `isDynamicallyReserved`. Since functions such as
> `isDynamicallyReserved` now requires "post-reservation-refinement"
> format, we must perform this conversion earlier.
> 
> In this patch, we use `upgradeResources` to perform resources
> validation __and__ convert the resources before going into the
> operation and task validation.
> 
> We really need a better plan for this going forward. MESOS-7702.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
>   src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
>   src/tests/resource_offers_tests.cpp 
> c2bbf834c1d46079af492887b9dd40e57f3f2ac7 
> 
> 
> Diff: https://reviews.apache.org/r/60283/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60377: Added test for reservation refinements and old agents.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [60377, 60374, 60284, 60255, 60283, 60352, 60351, 60282, 60281]

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

Error:
error: patch failed: src/master/master.cpp:4605
error: src/master/master.cpp: patch does not apply

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

- Mesos Reviewbot Windows


On June 22, 2017, 9:18 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60377/
> ---
> 
> (Updated June 22, 2017, 9:18 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the agent does not support reservation refinements, attempts to
> create a reservation refinement on the agent should be dropped by the
> master.
> 
> 
> Diffs
> -
> 
>   src/tests/upgrade_tests.cpp eef81018ab3fae2e91077d4314cdaba274111401 
> 
> 
> Diff: https://reviews.apache.org/r/60377/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`, ran ~5k iterations w/o error.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 22, 2017, 9:15 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated June 22, 2017, 9:15 p.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to verify that the state is recovered post reboot and
> agentId is retained given the recovery finishes without errors and
> if the recovery fails due to slaveInfo mismatch then agent no
> state is recoverd making the agent register as a new agent.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 90c172b6f7ca0082c3dfa2082050d6c18e4e636c 
> 
> 
> Diff: https://reviews.apache.org/r/56895/diff/10/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 60374: Required that resources are always downgraded for a non-capable agent.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On June 22, 2017, 7:09 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60374/
> ---
> 
> (Updated June 22, 2017, 7:09 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7575
> https://issues.apache.org/jira/browse/MESOS-7575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the master prevents refined reservations from being created on
> non-capable agents, we cannot be in a situation in which the resources
> are not downgradable for a non-capable agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b0818e1e59ce9804cd9592aa2a7ec8f80ba5bddf 
> 
> 
> Diff: https://reviews.apache.org/r/60374/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 60377: Added test for reservation refinements and old agents.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

If the agent does not support reservation refinements, attempts to
create a reservation refinement on the agent should be dropped by the
master.


Diffs
-

  src/tests/upgrade_tests.cpp eef81018ab3fae2e91077d4314cdaba274111401 


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


Testing
---

`make check`, ran ~5k iterations w/o error.


Thanks,

Neil Conway

Re: Review Request 60104: Added rebooted flag to State.

[Megha Sharma]

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

(Updated June 22, 2017, 9:17 p.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added rebooted flag to State to remember whether or not
the agent has rebooted.


Diffs
-

  src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
  src/slave/state.hpp 537358cb54393dd02b655050c26504808a1505ad 
  src/slave/state.cpp 5dd8b1cc29cf1809e948015e146691596a8202b8 


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


Testing (updated)
---

make check done together with 60105 and 56895


Thanks,

Megha Sharma

Re: Review Request 60105: Clean rebooted slave's state if slaveInfo mismatches.

[Megha Sharma]

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

(Updated June 22, 2017, 9:16 p.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.


Diffs
-

  src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 


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


Testing (updated)
---

make check done together with 60104 and 56895


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated June 22, 2017, 9:15 p.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


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


Repository: mesos


Description (updated)
---

Added tests to verify that the state is recovered post reboot and
agentId is retained given the recovery finishes without errors and
if the recovery fails due to slaveInfo mismatch then agent no
state is recoverd making the agent register as a new agent.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 90c172b6f7ca0082c3dfa2082050d6c18e4e636c 


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

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


Testing (updated)
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 60279: Add constructor for ObjectApprover::Object.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60279]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 22, 2017, 8:22 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60279/
> ---
> 
> (Updated June 22, 2017, 8:22 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add constructor for ObjectApprover::Object.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 95cbcf3f9a962a896c9c23e961f02eb16dff36f8 
>   src/common/http.cpp 2f7718cbc2e449b4f7c89754e8f84eac2f3c35b6 
>   src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 
>   src/slave/http.cpp cbbc1dc27cc90bac8d48cbbc84266c3d87490a3c 
> 
> 
> Diff: https://reviews.apache.org/r/60279/diff/2/
> 
> 
> Testing
> ---
> 
> Passed "make check -j48"
> No specific test case related.
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60281: Avoided sending an inconsistent `TaskInfo` to the allocator.

[Neil Conway]

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




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


This comment needs updating.


- Neil Conway


On June 21, 2017, 7:06 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60281/
> ---
> 
> (Updated June 21, 2017, 7:06 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since we drop invalid tasks on a `LAUNCH` operation, we send a new
> `Operation` with only the valid tasks appended. At the same time,
> we adjust the `TaskInfo` a little bit to perform validation, and
> to maintain backwards compatibility. However, the unmodified
> `TaskInfo` was sent to the allocator. Since the allocator does not
> look at the modified fields, this was inconsequential, but we should
> avoid sending inconsistent `TaskInfo`s in general.
> 
> We also avoided creating an unnecessary copy of the `TaskInfo`.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
> 
> 
> Diff: https://reviews.apache.org/r/60281/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60374: Required that resources are always downgraded for a non-capable agent.

[Neil Conway]

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




src/master/master.cpp
Line 4919 (original), 4907 (patched)


Can we add a comment explaining why these are `CHECK`s?


- Neil Conway


On June 22, 2017, 7:09 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60374/
> ---
> 
> (Updated June 22, 2017, 7:09 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7575
> https://issues.apache.org/jira/browse/MESOS-7575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the master prevents refined reservations from being created on
> non-capable agents, we cannot be in a situation in which the resources
> are not downgradable for a non-capable agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
> 
> 
> Diff: https://reviews.apache.org/r/60374/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60279: Add constructor for ObjectApprover::Object.

[Quinn Leng]

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

(Updated June 22, 2017, 8:22 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description (updated)
---

Add constructor for ObjectApprover::Object.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
95cbcf3f9a962a896c9c23e961f02eb16dff36f8 
  src/common/http.cpp 2f7718cbc2e449b4f7c89754e8f84eac2f3c35b6 
  src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 
  src/slave/http.cpp cbbc1dc27cc90bac8d48cbbc84266c3d87490a3c 


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

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


Testing
---

Passed "make check -j48"
No specific test case related.


Thanks,

Quinn Leng

Re: Review Request 60374: Required that resources are always downgraded for a non-capable agent.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60281, 60282, 60351, 60352, 60283, 60255, 60284, 60374]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 22, 2017, 7:09 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60374/
> ---
> 
> (Updated June 22, 2017, 7:09 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7575
> https://issues.apache.org/jira/browse/MESOS-7575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the master prevents refined reservations from being created on
> non-capable agents, we cannot be in a situation in which the resources
> are not downgradable for a non-capable agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
> 
> 
> Diff: https://reviews.apache.org/r/60374/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 59599: Added 'type' and 'name' fields to ResourceProviderInfo.

[Benjamin Bannier]

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



LGTM so far. Having some patch using this functionality immediately following 
would be great.

- Benjamin Bannier


On June 9, 2017, 1:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59599/
> ---
> 
> (Updated June 9, 2017, 1:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Chun-Hung Hsiao, 
> Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7560
> https://issues.apache.org/jira/browse/MESOS-7560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'type' and 'name' fields to ResourceProviderInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
> 
> 
> Diff: https://reviews.apache.org/r/59599/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 60374: Required that resources are always downgraded for a non-capable agent.

[Michael Park]

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

Review request for mesos and Neil Conway.


Repository: mesos


Description
---

Since the master prevents refined reservations from being created on
non-capable agents, we cannot be in a situation in which the resources
are not downgradable for a non-capable agent.


Diffs
-

  src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 60252: Fixed a bug that causes segfault in ProcessManager::finalize.

[Jiang Yan Xu]

> On June 21, 2017, 11:42 a.m., Joseph Wu wrote:
> > I agree this makes the finalization logic safer, but I don't see a way to 
> > add a `nullptr` to the `ProcessManager::processes` map.
> > 
> > `ProcessManager::spawn` is the only location where the `processes` map is 
> > inserted, and this method CHECK-fails if you give it a `nullptr`.  Two 
> > other locations use the map's `operator[]`, but those are guarded by a 
> > mutex so they will never empty-initialize a map entry.
> > 
> > Can you note how you ran into a segfault here?  It's more likely that the 
> > segfault was caused by a dereferencing a non-`nullptr` process that had 
> > already been deallocated (which would not be fixed by this review).
> 
> Jiang Yan Xu wrote:
> The use case is something like this: 
> 
> https://github.com/apache/mesos/blob/65152413836b58d01ace3a40bdc9056f9a489c6b/3rdparty/libprocess/src/tests/process_tests.cpp#L691
> 
> Arguably I made an error in doing:
> 
> ```
> ProcessBase process;
> UPID pid = spawn(&process, true);
> 
> ...
> 
> terminate(process);
> wait(process);
> ```
> 
> And the process (in a test method) goes out of scope before libprocess 
> finalize() so it segfaulted.
> 
> Just felt it's not really necessary to fail (and it used to not) in this 
> way?
> 
> Joseph Wu wrote:
> I see.  That error will still segfault, even with this patch.
> 
> ---
> 
> The tests (and otherwise invalid/risky uses of libprocess methods) did 
> not segfault in the past because we didn't clean up libprocess in the past.

Hmm you are right it's true that this wouldn't help. Discarding this. It'll 
probably not be much safer without using `shared_ptr`s.


- Jiang Yan


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


On June 20, 2017, 4:16 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60252/
> ---
> 
> (Updated June 20, 2017, 4:16 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We don't need and shouldn't assume pointers in `processes` are
> non-nullptr.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d 
> 
> 
> Diff: https://reviews.apache.org/r/60252/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60284: Removed unused `convertResourceFormat` for `Operation`.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60281, 60282, 60351, 60352, 60283, 60255, 60284]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 21, 2017, 12:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60284/
> ---
> 
> (Updated June 21, 2017, 12:08 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused `convertResourceFormat` for `Operation`.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.hpp 4b033687fa7224ffa36523cb3ca366f4384fd4f8 
>   src/common/resources_utils.cpp 751194c5ff5c794c77be3bcf16892af39de6d4d9 
> 
> 
> Diff: https://reviews.apache.org/r/60284/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.

[Neil Conway]

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




src/master/master.cpp
Line 4680 (original), 4698 (patched)


Not in this RR, but this seems dubious to me: if we're not able to launch a 
task with refined reservations on an old agent, we skip sending 
`RunTaskMessage` but we _do_ add the task to `operations`, which is later 
passed to the allocator. Seems wrong.

Also, do we have a test case for this situation?



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


I wonder if we want `validateAndUpgradeResources` of a `TaskInfo`.



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


Whitespace.



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


Can we remove this? Seems irrelevant to the test's purpose.


- Neil Conway


On June 21, 2017, 7:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60283/
> ---
> 
> (Updated June 21, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially, it seemed like calling `convertResourceFormat` after
> operation validation seemed safe since the operation validation
> themselves performed `Resources::validate` within them.
> 
> However, the rest of the operation validation code relies on
> the fact that the resources have been validated, and uses
> functions such as `isDynamicallyReserved`. Since functions such as
> `isDynamicallyReserved` now requires "post-reservation-refinement"
> format, we must perform this conversion earlier.
> 
> In this patch, we use `upgradeResources` to perform resources
> validation __and__ convert the resources before going into the
> operation and task validation.
> 
> We really need a better plan for this going forward. MESOS-7702.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
>   src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 
>   src/tests/resource_offers_tests.cpp 
> c2bbf834c1d46079af492887b9dd40e57f3f2ac7 
> 
> 
> Diff: https://reviews.apache.org/r/60283/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60369, 60370]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 22, 2017, 3:26 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 22, 2017, 3:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/1/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60351: Used `JSON::protobuf` to print resources not validated nor converted.

[Neil Conway]

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



I wonder if we should just change the definition of `operator<<` for 
`RepeatedPtrField`. The current behavior is to silently omit printing 
invalid resources, which seems very misleading. This would also avoid the risk 
of random `CHECK` failures if a code path attempts to print a resource before 
upgrading it.

- Neil Conway


On June 22, 2017, 12:08 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60351/
> ---
> 
> (Updated June 22, 2017, 12:08 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During the authorization phase, the resources have not been validated
> not converted to the "post-reservation-refinement" format.
> We can't rely on any operations that require valid resources and/or
> "post-reservation-refinement" format. `operator<<` is one of those
> functions, and here we print out the JSON representation of the
> resources instead.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
> 
> 
> Diff: https://reviews.apache.org/r/60351/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60282: Introduced `validateAndUpgradeResources`.

[Neil Conway]

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


Fix it, then Ship it!




"Validation code for operations, and tasks" in commit message: remove the comma.

"a `upgradeResources`" in commit message: "a `validateAndUpgradeResources`"


src/common/resources_utils.hpp
Lines 141 (patched)


"in their previous format" => "unchanged".


- Neil Conway


On June 21, 2017, 7:07 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60282/
> ---
> 
> (Updated June 21, 2017, 7:07 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validation code for operations, and tasks validated the resources as
> the first step, and assumed valid resources from then on. This means
> that it started to use functions such as `isDynamicallyReserved`.
> 
> However, since `isDynamicallyReserved` requires the resources to be
> in the "post-reservation-refinement" format, we must convert the
> resources before using those functions. For now, we introduce
> a `upgradeResources` abstraction which is called to validate and
> convert the resources before invoking operation / task validation.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.hpp 4b033687fa7224ffa36523cb3ca366f4384fd4f8 
>   src/common/resources_utils.cpp 751194c5ff5c794c77be3bcf16892af39de6d4d9 
> 
> 
> Diff: https://reviews.apache.org/r/60282/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60281: Added the adjusted `TaskInfo` into the `Operation` to be sent out.

[Neil Conway]

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


Ship it!




Can we clarify the commit message that the incorrect `TaskInfo` is actually 
being sent to the allocator? The agent gets the correct `TaskInfo`.

- Neil Conway


On June 21, 2017, 7:06 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60281/
> ---
> 
> (Updated June 21, 2017, 7:06 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since we drop invalid tasks on a `LAUNCH` operation, we send a new
> `Operation` with only the valid tasks appended. At the same time,
> we adjust the `TaskInfo` a little bit to perform validation, and
> to maintain backwards compatibility. In the end, we were appending
> the unadjusted task to the new `Operation` rather than the adjusted
> task. This patch changes this to append the adjusted task instead.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 
> 
> 
> Diff: https://reviews.apache.org/r/60281/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 59932: Added devolve functions for resource provider Event/Call.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On June 9, 2017, 1:12 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59932/
> ---
> 
> (Updated June 9, 2017, 1:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added devolve functions for resource provider Event/Call.
> 
> 
> Diffs
> -
> 
>   src/internal/devolve.hpp a8c500785f400f5933640ad6afcbb5a7dae796c0 
>   src/internal/devolve.cpp 73774d975aab39352cf6a215b9b47e02a4b0aabf 
> 
> 
> Diff: https://reviews.apache.org/r/59932/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60280]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 22, 2017, 5:20 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 22, 2017, 5:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/3/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 59601: Removed an unused declaration in type_utils.hpp.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On June 9, 2017, 1:10 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59601/
> ---
> 
> (Updated June 9, 2017, 1:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Joseph Wu, Jan 
> Schlicht, and Zhongbo Tian.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unused declaration in type_utils.hpp.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
> 
> 
> Diff: https://reviews.apache.org/r/59601/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 60346: Improved the documentation of 'TASK_LOST'.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60346]

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 June 21, 2017, 10:17 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60346/
> ---
> 
> (Updated June 21, 2017, 10:17 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7662
> https://issues.apache.org/jira/browse/MESOS-7662
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the protobuf comments to clarify that 'TASK_LOST' is not
> always a terminal state.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 38449da44a5288afc7e2ef17ef5e1380ecabd5d2 
>   include/mesos/v1/mesos.proto 81eca47ce1074f5e522a0410f01ab2b2cfc76aa2 
> 
> 
> Diff: https://reviews.apache.org/r/60346/diff/1/
> 
> 
> Testing
> ---
> 
> None, this is not functional change.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 60369: Exposed allocated resources per each role in /state endpoint on agent.

[Andrei Budnik]

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

Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

The JSON key for this information is "reserved_resources_allocated"
and "unreserved_resources_allocated".


Diffs
-

  src/slave/http.cpp cbbc1dc27cc90bac8d48cbbc84266c3d87490a3c 
  src/tests/reservation_endpoints_tests.cpp 
f710a188a7875c1cb847e39276b4b65332703ca5 


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


Testing
---

make check


Thanks,

Andrei Budnik

Review Request 60370: Updated agent webui page to display allocated resources per each role.

[Andrei Budnik]

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

Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

Updated agent webui page to display allocated resources per each role.


Diffs
-

  src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
  src/webui/master/static/js/controllers.js 
67bfd030649dd21840c16188a4964f814aa232d7 


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


Testing
---

See screenshot.


File Attachments


agent.html
  
https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png


Thanks,

Andrei Budnik

Re: Review Request 58394: Setup new directory for python http client lib in src/python.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [58394]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On May 11, 2017, 7:34 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58394/
> ---
> 
> (Updated May 11, 2017, 7:34 p.m.)
> 
> 
> Review request for mesos, Jason Lai, Joseph Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7310
> https://issues.apache.org/jira/browse/MESOS-7310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/58394
> 
> 
> Diffs
> -
> 
>   src/cli_new/bootstrap 6d62e9adf1d543ed00a3a2cf2484edf1c33ee443 
>   src/python/.gitignore PRE-CREATION 
>   src/python/lib/mesos/__init__.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58394/diff/2/
> 
> 
> Testing
> ---
> 
> Under src/cli_new:
> 1\. ./bootstrap
> 2\. . ./activate
> 3\. python
> 4\. >>> import mesos
> 5\. >>> mesos.\_\_path\_\_
> 6\. verify that the path printed out is indeed at src/python/lib/mesos
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

[Aaron Wood via Review Board]

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

(Updated June 22, 2017, 3:20 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


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


Repository: mesos


Description (updated)
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


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


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

[Aaron Wood via Review Board]

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

(Updated June 22, 2017, 3:19 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


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


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. Additionally, the name of the binary is never passed 
as an argument so executors making use of argv[0] will fail. This provides the 
full path to the executor so that the `execvp` or `execvpe` is successful. The 
name of the binary is also passed as the first argument for cases where there 
is no shell used.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


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

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


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 58394: Setup new directory for python http client lib in src/python.

[Kevin Klues]

> On May 2, 2017, 11:11 p.m., Kevin Klues wrote:
> > src/python/lib/mesos/__init__.py
> > Lines 1 (patched)
> > 
> >
> > I would exclude this from this commit.
> > It is not used anywhere, therefore it should not be included until the 
> > commit where it is used.
> 
> Eric Chung wrote:
> this makes it clear that this dir is a python package, in addition to 
> making git check the dir in. i would like to keep it unless there is a very 
> strong reason not to.

I just meant to remove the line for the absolute_import. I understand the need 
for the file in general.


- Kevin


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


On May 11, 2017, 6:34 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58394/
> ---
> 
> (Updated May 11, 2017, 6:34 p.m.)
> 
> 
> Review request for mesos, Jason Lai, Joseph Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7310
> https://issues.apache.org/jira/browse/MESOS-7310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/58394
> 
> 
> Diffs
> -
> 
>   src/cli_new/bootstrap 6d62e9adf1d543ed00a3a2cf2484edf1c33ee443 
>   src/python/.gitignore PRE-CREATION 
>   src/python/lib/mesos/__init__.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58394/diff/2/
> 
> 
> Testing
> ---
> 
> Under src/cli_new:
> 1\. ./bootstrap
> 2\. . ./activate
> 3\. python
> 4\. >>> import mesos
> 5\. >>> mesos.\_\_path\_\_
> 6\. verify that the path printed out is indeed at src/python/lib/mesos
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 60107: Add class definition for ObjectFilter.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60107]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 20, 2017, 12:30 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> ---
> 
> (Updated June 20, 2017, 12:30 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add class definition for ObjectFilter.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 48f7c4a9c35371544c92bb1712e1e354feb20987 
>   src/common/http.cpp 98899a789610add924a7d2c0af943230cf914d2d 
> 
> 
> Diff: https://reviews.apache.org/r/60107/diff/4/
> 
> 
> Testing
> ---
> 
> Passed "make check"
> No specific test case related
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60107: Add class definition for ObjectFilter.

[Alexander Rojas]

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



Also, I don't know if calling it a filter is the right thing to do. A filter 
should recieve a data set and remove things from it. This abstraction can only 
verify one element at a time, therefore it is not a filter. Acceptor would be a 
better name for it.

- Alexander Rojas


On June 20, 2017, 2:30 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> ---
> 
> (Updated June 20, 2017, 2:30 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add class definition for ObjectFilter.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 48f7c4a9c35371544c92bb1712e1e354feb20987 
>   src/common/http.cpp 98899a789610add924a7d2c0af943230cf914d2d 
> 
> 
> Diff: https://reviews.apache.org/r/60107/diff/4/
> 
> 
> Testing
> ---
> 
> Passed "make check"
> No specific test case related
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60124: Added a name to ballon framework to distinguish between instances.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [60124, 60123, 60122]

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

Error:
error: patch failed: src/examples/balloon_framework.cpp:494
error: src/examples/balloon_framework.cpp: patch does not apply

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

- Mesos Reviewbot Windows


On June 15, 2017, 7:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60124/
> ---
> 
> (Updated June 15, 2017, 7:09 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp bfaae3796afb426459ef5cff3a37634a8242032e 
> 
> 
> Diff: https://reviews.apache.org/r/60124/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 60124: Added a name to ballon framework to distinguish between instances.

[Alexander Rukletsov]

> On June 15, 2017, 7:07 p.m., Armand Grillet wrote:
> > src/examples/balloon_framework.cpp
> > Line 464 (original), 469 (patched)
> > 
> >
> > Consistency wise, other tests have just `executor.set_name("Framework 
> > Name Executor (C++)");` to set the name of the executor (e.g. in 
> > `long_lived_framework.cpp`). More generally, what is the reason to add a 
> > `name` flag for this test?
> 
> Alexander Rukletsov wrote:
> If we have multiple instance of the same framework, it can be difficult 
> to distinguish between them in the WebUI, hence this change. I agree, this is 
> not only relevant to this framework, but actually to all example frameworks.
> 
> Joseph Wu wrote:
> You might as well let the flag control the entire name, rather than 
> appending `--name` to the end of `Balloon Framework`.  Or update the flag 
> description to mention that it will be appended to the canonical framework 
> name.

Good idea, Joseph.


- Alexander


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


On June 15, 2017, 5:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60124/
> ---
> 
> (Updated June 15, 2017, 5:09 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp bfaae3796afb426459ef5cff3a37634a8242032e 
> 
> 
> Diff: https://reviews.apache.org/r/60124/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 60279: Add constructor for ObjectApprover::Object.

[Alexander Rojas]

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




src/common/http.cpp
Lines 815-818 (original), 815-817 (patched)


you could also do:

```c++
Try approved = 
frameworksApprover->approved(ObjectApprover::Object(&frameworkInfo));
```

here and everywhere else.


- Alexander Rojas


On June 21, 2017, 8:35 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60279/
> ---
> 
> (Updated June 21, 2017, 8:35 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add constructor for ObjectApprover::Object.
> 
> Add constructors for ObjectApprover::Object, making the caller side cleaner.
> Only one case is ignored, it's in "src/slave/slave.cpp:6709", since it's 
> setting parameters after the construction.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 95cbcf3f9a962a896c9c23e961f02eb16dff36f8 
>   src/common/http.cpp 2f7718cbc2e449b4f7c89754e8f84eac2f3c35b6 
>   src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 
>   src/slave/http.cpp cbbc1dc27cc90bac8d48cbbc84266c3d87490a3c 
> 
> 
> Diff: https://reviews.apache.org/r/60279/diff/1/
> 
> 
> Testing
> ---
> 
> Passed "make check -j48"
> No specific test case related.
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

[Jie Yu]

> On June 21, 2017, 11:07 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 671-674 (patched)
> > 
> >
> > Sorry, I got confused. User should be the one setting argv[0]. So we 
> > don't need to change the code here.
> 
> Aaron Wood wrote:
> No problem.
> So every time someone wants to make use of `argv[0]` in their executor 
> somewhere they'll have to construct that argument and send it along in the 
> protos. Why not take care of it here so that it's less to do (and less going 
> over the wire) from the scheduler's side?

Some user might want to customize argv[0] (which will be displayed by ps).


- Jie


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


On June 21, 2017, 10:26 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/2/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60162: Fixed bug in GroupTest.ConnectTimer, GroupTest.TimerCleanup.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60161, 60162]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 16, 2017, 8:26 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60162/
> ---
> 
> (Updated June 16, 2017, 8:26 p.m.)
> 
> 
> Review request for mesos and Andrei Budnik.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These tests looked for dispatches to `GroupProces::expired` as a way to
> determine when the current Zk session has expired. However, the previous
> implementation of the connect timer (`GroupProcess::timedout`) invoked
> `GroupProcess::expired` directly, which meant that an `EXPECT_DISPATCH`
> on the `expired` method should not fire.
> 
> However, a separate bug in `EXPECT_DISPATCH` (MESOS-5886) meant that the
> test expectations were actually being satisfied by a dispatch to
> `GroupProcess::timedout`, which meant the tests happened to work (!).
> 
> Fix this by changing `GroupProcess::timedout` to dispatch to `expired`
> rather than invoking it directly.
> 
> 
> Diffs
> -
> 
>   src/zookeeper/group.cpp 20f1928e05123d362147ce4ab0d6a752d95e466d 
> 
> 
> Diff: https://reviews.apache.org/r/60162/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Validated that if the fix for MESOS-5886 is applied without this change, 
> `GroupTest.ConnectTimer` fails. With this change, the test passes (both with 
> and without the fix for MESOS-5886 applied).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60161: Cleaned up zookeeper binding code slightly.

[Andrei Budnik]

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


Ship it!




Ship It!

- Andrei Budnik


On June 16, 2017, 6:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60161/
> ---
> 
> (Updated June 16, 2017, 6:23 p.m.)
> 
> 
> Review request for mesos and Andrei Budnik.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up zookeeper binding code slightly.
> 
> 
> Diffs
> -
> 
>   src/zookeeper/detector.cpp 973f5f297208dac2050d38fc5140a78c244666b7 
>   src/zookeeper/group.cpp 20f1928e05123d362147ce4ab0d6a752d95e466d 
> 
> 
> Diff: https://reviews.apache.org/r/60161/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60162: Fixed bug in GroupTest.ConnectTimer, GroupTest.TimerCleanup.

[Andrei Budnik]

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


Ship it!




Ship It!

- Andrei Budnik


On June 16, 2017, 6:26 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60162/
> ---
> 
> (Updated June 16, 2017, 6:26 p.m.)
> 
> 
> Review request for mesos and Andrei Budnik.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These tests looked for dispatches to `GroupProces::expired` as a way to
> determine when the current Zk session has expired. However, the previous
> implementation of the connect timer (`GroupProcess::timedout`) invoked
> `GroupProcess::expired` directly, which meant that an `EXPECT_DISPATCH`
> on the `expired` method should not fire.
> 
> However, a separate bug in `EXPECT_DISPATCH` (MESOS-5886) meant that the
> test expectations were actually being satisfied by a dispatch to
> `GroupProcess::timedout`, which meant the tests happened to work (!).
> 
> Fix this by changing `GroupProcess::timedout` to dispatch to `expired`
> rather than invoking it directly.
> 
> 
> Diffs
> -
> 
>   src/zookeeper/group.cpp 20f1928e05123d362147ce4ab0d6a752d95e466d 
> 
> 
> Diff: https://reviews.apache.org/r/60162/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Validated that if the fix for MESOS-5886 is applied without this change, 
> `GroupTest.ConnectTimer` fails. With this change, the test passes (both with 
> and without the fix for MESOS-5886 applied).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60353: Allowed dashes in Python modules.

[Armand Grillet]

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




src/cli_new/pylint.config
Lines 16 (patched)


I do not think that having a pylint.config for all our Python directories 
in src/cli_new is the most logical solution. We could move pylint.config to 
support/ in this review request or add a new PyLint configuration, including a 
new pylint.config, for these support scripts. It will require a change in the 
`run_lint` method of `PyLinter` to use different settings when linting the 
directories.

I would go for the second solution due to some errors currently returned by 
PyLint when linting files such as support/post-reviews.py, e.g. `E: 53, 0: No 
name 'version' in module 'distutils' (no-name-in-module)` that is likely due to 
the `virtualenv_dir` used by PyLint (the one used for `cli_new`). We are likely 
need at least two separate settings depending on what we lint: a standard 
Python module or a list of scripts.


- Armand Grillet


On June 22, 2017, 12:57 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60353/
> ---
> 
> (Updated June 22, 2017, 12:57 a.m.)
> 
> 
> Review request for mesos, Armand Grillet and Benjamin Bannier.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> None of our support scripts are intended to be used as modules, so
> the usage of dashes in the filenames are acceptable.  PyLint however
> considers dashes in module names (same as the filename) to be an error.
> 
> This commit also adds the `support` directory to the list of linted
> sources.
> 
> 
> Diffs
> -
> 
>   src/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60353/diff/1/
> 
> 
> Testing
> ---
> 
> This shouldn't be commited yet as a few support scripts don't pass the linter 
> yet.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>