Re: Review Request 51592: Added a debug logging for a CHECK failure in MountInfoTable::read.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51592]

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 Sept. 2, 2016, 1:11 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51592/
> ---
> 
> (Updated Sept. 2, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-6118
> https://issues.apache.org/jira/browse/MESOS-6118
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Looks like this CHECK can fail on some systems under load. See the ticket for 
> details.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 0d025d31a2947920517f9c8abfc14dd4af0c73d0 
> 
> Diff: https://reviews.apache.org/r/51592/diff/
> 
> 
> Testing
> ---
> 
> trivial.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 50693: Added another function `createSlaveInfo` for allocator benchmark test.

[Guangya Liu]

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

(Updated 九月 2, 2016, 2:33 a.m.)


Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.


Repository: mesos


Description
---

In allocator benchmark test, when `addSlave`, the test will first
create slave info, but currently, the parameter for `createSlaveInfo`
is a resource string, and this caused the `createSlaveInfo` will
always parse resource first before set resource for the agent. This
caused the time of adding agent is not accurate.

This fix is adding another function named as `createSlaveInfo` but
taking `Resources` as input parameter, this will remove the time
of parsing resources when setting resource for the agent and thus
making the time of adding agent more accurate.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
d960b7575ed5531753e9329e5774b6909090edf8 

Diff: https://reviews.apache.org/r/50693/diff/


Testing
---

make
make check

Before fix adding 3 agents.
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
Set quota for 1 roles in 1216us
Added 1 frameworks in 509us
Added 3 agents in 14.515326secs
/metrics/snapshot took 48615us for 3 agents and 1 frameworks
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14679 
ms)
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14679 ms total)
```

After fix adding 3 agents.
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
Set quota for 1 roles in 1238us
Added 1 frameworks in 555us
Added 3 agents in 13.976131secs
/metrics/snapshot took 58360us for 3 agents and 1 frameworks
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14139 
ms)
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14140 ms total)
```


Thanks,

Guangya Liu

Re: Review Request 50693: Added another function `createSlaveInfo` for allocator benchmark test.

[Guangya Liu]

> On 九月 1, 2016, 8:50 p.m., Jiang Yan Xu wrote:
> > The rationale isn't clear to me: for the same test if we always parse the 
> > resources at the same place, isn't the measurement still accurate when you 
> > compare the benchmark results with vs. without certain patches or between 
> > releases? The absolute numbers don't matter too much as they are not 
> > indicative of "real" workloads anyways right?
> > 
> > I am OK with adding this method though and Anindya has another patch 
> > https://reviews.apache.org/r/49571/ that does this because we need to 
> > construct more complex resources. Will it be OK to just have this method in 
> > https://reviews.apache.org/r/49571/?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, what about make the patch of 
> https://reviews.apache.org/r/49571/ focus on shared resource benchmark test 
> while this one focus on the new API for `createSlaveInfo`? The 
> `createSlaveInfo` will involve many changes and handling this in a separate 
> patch maybe better?
> 
> Jiang Yan Xu wrote:
> By "will involve many changes" do you mean you'd like to change many/all 
> call-sites to use the new method? I am just saying I don't see why we need to 
> change the existing call-sites for "benchmark accuracy concerns". 
> 
> How about in this patch we just add this method itself?

The reason is that I do not want to let the benchmark test to call old 
`createSlaveInfo` as this will call `Resources::parse` each time when adding a 
new agent. So I was only updating the caller part for the `benchmark` test, 
make sense?


- Guangya


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


On 八月 26, 2016, 7:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50693/
> ---
> 
> (Updated 八月 26, 2016, 7:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In allocator benchmark test, when `addSlave`, the test will first
> create slave info, but currently, the parameter for `createSlaveInfo`
> is a resource string, and this caused the `createSlaveInfo` will
> always parse resource first before set resource for the agent. This
> caused the time of adding agent is not accurate.
> 
> This fix is adding another function named as `createSlaveInfo` but
> taking `Resources` as input parameter, this will remove the time
> of parsing resources when setting resource for the agent and thus
> making the time of adding agent more accurate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/50693/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> Before fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1216us
> Added 1 frameworks in 509us
> Added 3 agents in 14.515326secs
> /metrics/snapshot took 48615us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14679 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14679 ms total)
> ```
> 
> After fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1238us
> Added 1 frameworks in 555us
> Added 3 agents in 13.976131secs
> /metrics/snapshot took 58360us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14139 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14140 ms total)
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 51592: Added a debug logging for a CHECK failure in MountInfoTable::read.

[Jie Yu]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

Looks like this CHECK can fail on some systems under load. See the ticket for 
details.


Diffs
-

  src/linux/fs.cpp 0d025d31a2947920517f9c8abfc14dd4af0c73d0 

Diff: https://reviews.apache.org/r/51592/diff/


Testing
---

trivial.


Thanks,

Jie Yu

Re: Review Request 51096: Added the `mesos-cni-port-mapper` binary.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 30, 2016, 3:54 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51096/
> ---
> 
> (Updated Aug. 30, 2016, 3:54 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `mesos-cni-port-mapper` is a CNI plugin that can be used to install
> port-forwarding rules for a container. The `mesos-cni-port-mapper` is a
> wrapper CNI plugin that should always be used in conjunction with
> other CNI plugins.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51096/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 50693: Added another function `createSlaveInfo` for allocator benchmark test.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (line 239)


Have this method call the other one?


- Jiang Yan Xu


On Aug. 26, 2016, 12:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50693/
> ---
> 
> (Updated Aug. 26, 2016, 12:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In allocator benchmark test, when `addSlave`, the test will first
> create slave info, but currently, the parameter for `createSlaveInfo`
> is a resource string, and this caused the `createSlaveInfo` will
> always parse resource first before set resource for the agent. This
> caused the time of adding agent is not accurate.
> 
> This fix is adding another function named as `createSlaveInfo` but
> taking `Resources` as input parameter, this will remove the time
> of parsing resources when setting resource for the agent and thus
> making the time of adding agent more accurate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/50693/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> Before fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1216us
> Added 1 frameworks in 509us
> Added 3 agents in 14.515326secs
> /metrics/snapshot took 48615us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14679 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14679 ms total)
> ```
> 
> After fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1238us
> Added 1 frameworks in 555us
> Added 3 agents in 13.976131secs
> /metrics/snapshot took 58360us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14139 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14140 ms total)
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50693: Added another function `createSlaveInfo` for allocator benchmark test.

[Guangya Liu]

> On 九月 1, 2016, 8:50 p.m., Jiang Yan Xu wrote:
> > The rationale isn't clear to me: for the same test if we always parse the 
> > resources at the same place, isn't the measurement still accurate when you 
> > compare the benchmark results with vs. without certain patches or between 
> > releases? The absolute numbers don't matter too much as they are not 
> > indicative of "real" workloads anyways right?
> > 
> > I am OK with adding this method though and Anindya has another patch 
> > https://reviews.apache.org/r/49571/ that does this because we need to 
> > construct more complex resources. Will it be OK to just have this method in 
> > https://reviews.apache.org/r/49571/?

Thanks Jiang Yan, what about make the patch of 
https://reviews.apache.org/r/49571/ focus on shared resource benchmark test 
while this one focus on the new API for `createSlaveInfo`? The 
`createSlaveInfo` will involve many changes and handling this in a separate 
patch maybe better?


- Guangya


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


On 八月 26, 2016, 7:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50693/
> ---
> 
> (Updated 八月 26, 2016, 7:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In allocator benchmark test, when `addSlave`, the test will first
> create slave info, but currently, the parameter for `createSlaveInfo`
> is a resource string, and this caused the `createSlaveInfo` will
> always parse resource first before set resource for the agent. This
> caused the time of adding agent is not accurate.
> 
> This fix is adding another function named as `createSlaveInfo` but
> taking `Resources` as input parameter, this will remove the time
> of parsing resources when setting resource for the agent and thus
> making the time of adding agent more accurate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/50693/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> Before fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1216us
> Added 1 frameworks in 509us
> Added 3 agents in 14.515326secs
> /metrics/snapshot took 48615us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14679 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14679 ms total)
> ```
> 
> After fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1238us
> Added 1 frameworks in 555us
> Added 3 agents in 13.976131secs
> /metrics/snapshot took 58360us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14139 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14140 ms total)
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50693: Added another function `createSlaveInfo` for allocator benchmark test.

[Jiang Yan Xu]

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



The rationale isn't clear to me: for the same test if we always parse the 
resources at the same place, isn't the measurement still accurate when you 
compare the benchmark results with vs. without certain patches or between 
releases? The absolute numbers don't matter too much as they are not indicative 
of "real" workloads anyways right?

I am OK with adding this method though and Anindya has another patch 
https://reviews.apache.org/r/49571/ that does this because we need to construct 
more complex resources. Will it be OK to just have this method in 
https://reviews.apache.org/r/49571/?

- Jiang Yan Xu


On Aug. 26, 2016, 12:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50693/
> ---
> 
> (Updated Aug. 26, 2016, 12:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In allocator benchmark test, when `addSlave`, the test will first
> create slave info, but currently, the parameter for `createSlaveInfo`
> is a resource string, and this caused the `createSlaveInfo` will
> always parse resource first before set resource for the agent. This
> caused the time of adding agent is not accurate.
> 
> This fix is adding another function named as `createSlaveInfo` but
> taking `Resources` as input parameter, this will remove the time
> of parsing resources when setting resource for the agent and thus
> making the time of adding agent more accurate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/50693/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> Before fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1216us
> Added 1 frameworks in 509us
> Added 3 agents in 14.515326secs
> /metrics/snapshot took 48615us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14679 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14679 ms total)
> ```
> 
> After fix adding 3 agents.
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32
> Set quota for 1 roles in 1238us
> Added 1 frameworks in 555us
> Added 3 agents in 13.976131secs
> /metrics/snapshot took 58360us for 3 agents and 1 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/32 (14139 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (14140 ms total)
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 51566: Changed Master::_accept() to not pass invalid tasks to the allocator.

[Anindya Sinha]

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


Ship it!




Ship It!

- Anindya Sinha


On Aug. 31, 2016, 10:42 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51566/
> ---
> 
> (Updated Aug. 31, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Anindya Sinha.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> So in Allocator::updateAllocation() we can assume all tasks are valid.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 192b2ce5eb24deb6e843f2f1a8c915114614b268 
> 
> Diff: https://reviews.apache.org/r/51566/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 45964: Add unit tests for sharing of resources.

[Jiang Yan Xu]

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



Moved over some tests to their corresponding changes:

https://github.com/apache/mesos/commit/d4970b78313943aecff7d0366621fbab43563c84
https://github.com/apache/mesos/commit/1d5ea3d48e95a10ba6064d9560718f75d14447fc

Moved in tests from https://reviews.apache.org/r/45961/


src/tests/resources_tests.cpp (line 790)






src/tests/resources_tests.cpp (line 790)


Changed this to

```
TEST(SharedResourcesTest, Printing)
{
  Resources volume = createPersistentVolume(
  Megabytes(64),
  "role1",
  "id1",
  "path1",
  None(),
  None(),
  "principal1",
  true); // Shared.

  {
ostringstream oss;

oss << volume;
EXPECT_EQ("disk(role1)[id1:path1]:64<1>", oss.str());
  }

  {
ostringstream oss;

oss << volume + volume;
EXPECT_EQ("disk(role1)[id1:path1]:64<2>", oss.str());
  }
}
```

And committed in 
https://github.com/apache/mesos/commit/1d5ea3d48e95a10ba6064d9560718f75d14447fc



src/tests/resources_tests.cpp (lines 2363 - 2369)


Might be worth adding a stanza that DESTROY the volume and the resource 
should be back to the original.

I added it to the commit: 
https://github.com/apache/mesos/blob/02acd84bfb96917c36af5f7c7813fd9d708977df/src/tests/resources_tests.cpp#L2326-L2332



src/tests/resources_tests.cpp (lines 2548 - 2565)


`shared()` is covered in `SharedResourcesTest.Filter`. What do these lines 
add in terms of test coverage?


- Jiang Yan Xu


On Aug. 29, 2016, 12:22 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45964/
> ---
> 
> (Updated Aug. 29, 2016, 12:22 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4324
> https://issues.apache.org/jira/browse/MESOS-4324
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add unit tests for sharing of resources.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ddd48694e5c2d3d01fbff20558a46c1256b3dbc3 
>   src/tests/master_validation_tests.cpp 
> e440ff3f6d04f797e65874b9f610ed63d9f28e0e 
>   src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
>   src/tests/persistent_volume_tests.cpp 
> a6f97c4bb5fb29d610c01255036095e2b30c44c5 
>   src/tests/resources_tests.cpp 4932d95f4e78cae764b89472373e13527b4354a2 
> 
> Diff: https://reviews.apache.org/r/45964/diff/
> 
> 
> Testing
> ---
> 
> Tests for shared resources added for allocator, resources and sorter.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51553: Changed the way `HAP::updateAllocation()` calls `Resources.apply()`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51566, 51553]

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 Sept. 1, 2016, 4:35 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51553/
> ---
> 
> (Updated Sept. 1, 2016, 4:35 p.m.)
> 
> 
> Review request for mesos and Anindya Sinha.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is needed for supporting LAUNCH operations with shared resources
> (More details in https://reviews.apache.org/r/45961/) but this patch
> itself has no functional change.
> 
> Also, this patch together with https://reviews.apache.org/r/51412/
> results in no change in the granularity in which Offer operations are
> applied. Each single operation in the ACCEPT call is still individually
> applied to Resources objects managed by the master, the allocator and
> the sorters, same as before.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51553/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Aug. 26, 2016, 6:24 a.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 275
> > 
> >
> > I think we should use `insert(...)` instead of `=`; if the following 
> > event in the queue, are we still going to get the correct result?
> > ```
> > batch -> addFramework(f1) -> addFramework(f2)
> > ```
> 
> Jacob Janco wrote:
> insert in a loop?
> 
> Klaus Ma wrote:
> No; if multiple frameworks register at almost the same time, will there 
> be multiple addFramework events pending in the queue? Did not get a chance to 
> test it, but did we consider such kind of race condition?

If I understand this correctly: batch -> addFramework(f1) -> addFramework(f2) 
will ensure an allocate() at batch over all known slaves. If the two 
addFramework events come in close succession then the queue should look like 1: 
addFramework(f1) -> addFramework(f2) 2: addFramework(f2) -> allocate() 3: 
allocate() There will be a pendingAllocation set for the first event with the 
dispatched allocate() after addFramework(f2). I'm not sure what the race 
condition would be as we can have many addFramework events queued.


- Jacob


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


On Aug. 30, 2016, 4:53 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 30, 2016, 4:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> - Batched allocations are handled synchronously.
> 
>   
> - Carrying over work from 
>   https://reviews.apache.org/r/41658/ and added 
>   the previous reviewers
> - Specifically, this patch introduces the boolean 
>   flag pendingAllocation, which when set on event 
>   triggered allocations, will prevent additional 
>   no-op allocations: the flag is cleared when the 
>   enqueued allocation is processed, subsequent 
>   event triggered allocations will update a set
>   of allocation candidates rather than dispatching 
>   an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51560: Deprecated using health checks without setting the type.

[haosdent huang]

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




src/health-check/health_checker.cpp (lines 178 - 186)


We don't need to care the compatibility with the old HTTP Health Check 
protobuf here because custom executor would not use `HealthCheckerProcess`.


- haosdent huang


On Sept. 1, 2016, 6:18 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51560/
> ---
> 
> (Updated Sept. 1, 2016, 6:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
> 
> 
> Bugs: MESOS-6110
> https://issues.apache.org/jira/browse/MESOS-6110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Deprecated using health checks without setting the type.
> 
> 
> Diffs
> -
> 
>   CHANGELOG eadf546f96e4f8f83107de3e1421197875a46e2b 
>   docs/upgrades.md dc43cad4d9e0bec0f4017ac53d740687c060b332 
>   include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
>   include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
>   src/health-check/health_checker.cpp 
> f373df19fc8af8e9650be61e3b101e89362a67cd 
> 
> Diff: https://reviews.apache.org/r/51560/diff/
> 
> 
> Testing
> ---
> 
> Could verfied from 
> https://github.com/haosdent/mesos/blob/85abb6da058bf8daf4e794df7da8eb5f240b671c/docs/upgrades.md
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51561: Fixed the overview table style in upgrades.md.

[haosdent huang]

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

(Updated Sept. 1, 2016, 6:18 p.m.)


Review request for mesos, Alexander Rukletsov and Joseph Wu.


Changes
---

Rebase.


Repository: mesos


Description
---

Fixed the overview table style in upgrades.md.


Diffs (updated)
-

  docs/upgrades.md dc43cad4d9e0bec0f4017ac53d740687c060b332 

Diff: https://reviews.apache.org/r/51561/diff/


Testing
---

Could verfied from 
https://github.com/haosdent/mesos/blob/85abb6da058bf8daf4e794df7da8eb5f240b671c/docs/upgrades.md


Thanks,

haosdent huang

Re: Review Request 51560: Deprecated using health checks without setting the type.

[haosdent huang]

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

(Updated Sept. 1, 2016, 6:18 p.m.)


Review request for mesos, Alexander Rukletsov and Joseph Wu.


Changes
---

Fix comments.


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


Repository: mesos


Description
---

Deprecated using health checks without setting the type.


Diffs (updated)
-

  CHANGELOG eadf546f96e4f8f83107de3e1421197875a46e2b 
  docs/upgrades.md dc43cad4d9e0bec0f4017ac53d740687c060b332 
  include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
  include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
  src/health-check/health_checker.cpp f373df19fc8af8e9650be61e3b101e89362a67cd 

Diff: https://reviews.apache.org/r/51560/diff/


Testing
---

Could verfied from 
https://github.com/haosdent/mesos/blob/85abb6da058bf8daf4e794df7da8eb5f240b671c/docs/upgrades.md


Thanks,

haosdent huang

Re: Review Request 45961: Support sharing of resources through reference counting of resources.

[Jiang Yan Xu]

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



Updated the review with the following changes.

I have already committed all 'resources*.cpp's so they are removed from the 
review. 
I moved the tests to /r/45962/ so the remainder of the current set of tests are 
in one place.

Please take a look.


src/master/allocator/mesos/hierarchical.cpp (lines 620 - 621)






src/master/allocator/mesos/hierarchical.cpp (lines 622 - 630)


I am a bit uncomfortable with processing LAUNCHes after all other 
operations are applied. This may or may not be problematic but it's not the 
same as the original order. Implementation cleanliness aside, I think it's most 
intuitive if we structure it as "loop through the list of operations and handle 
them individually, we just added some logic for handling the LAUNCH operation".

In fact in my change I kept most of the method unchanged.



src/master/allocator/mesos/hierarchical.cpp (lines 651 - 653)






src/master/allocator/mesos/hierarchical.cpp (line 693)


`additional` could have more instances than `updatedFrameworkAllocation`.



src/master/allocator/sorter/drf/sorter.cpp (lines 163 - 178)


I changed it to the following which I think is cleaner and more direct 
translation of the comment.

```
  // Add shared resources to the total quantities when the same
  // resources don't already exist in the allocation.
  const Resources newShared = resources.shared().filter([this, name, 
slaveId](
  const Resource& resource) {
return !allocations[name].resources[slaveId].contains(resource);
  });

  allocations[name].resources[slaveId] += resources;
  allocations[name].scalarQuantities +=
(resources.nonShared() + newShared).createStrippedScalarQuantity();
```



src/master/allocator/sorter/drf/sorter.cpp (lines 285 - 296)


Similarly:

```
  // Remove shared resources from the allocated quantities when there
  // are no instances of same resources left in the allocation.
  const Resources absentShared = resources.shared().filter(
  [this, name, slaveId](const Resource& resource) {
return !allocations[name].resources[slaveId].contains(resource);
  });

  const Resources resourcesQuantity =
(resources.nonShared() + absentShared).createStrippedScalarQuantity();
```



src/master/allocator/sorter/drf/sorter.cpp (lines 314 - 328)


Similarly,

```
// Add shared resources to the total quantities when the same
// resources don't already exist in the total.
const Resources newShared = resources.shared().filter([this, slaveId](
const Resource& resource) {
  return !total_.resources[slaveId].contains(resource);
});

total_.resources[slaveId] += resources;
total_.scalarQuantities +=
  (resources.nonShared() + newShared).createStrippedScalarQuantity();

```



src/master/allocator/sorter/drf/sorter.cpp (lines 347 - 359)


Similarly,

```
// Remove shared resources from the total quantities when there
// are no instances of same resources left in the total.
const Resources absentShared = resources.shared().filter(
[this, slaveId](const Resource& resource) {
  return !total_.resources[slaveId].contains(resource);
});

const Resources resourcesQuantity =
  (resources.nonShared() + absentShared).createStrippedScalarQuantity();
```



src/master/master.cpp (lines 3807 - 3808)


Why the change from `offeredSharedResources = _offeredResources.shared();`?

This won't work if `offeredSharedResources` has multiple copies.

I also changed the CREATE case to be more consistent.



src/master/master.cpp (lines 3820 - 3821)


This is far away from its first usage. Plus the comments about why we are 
doing it is below. I moved it down.



src/master/master.cpp (line 3821)


Indentation.



src/master/master.cpp (line 6636)


Kill this redundant line.



src/master/validation.cpp (line 904)


I moved 

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51525]

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 Sept. 1, 2016, 3:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51525/
> ---
> 
> (Updated Sept. 1, 2016, 3:28 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds tooling to both build and execute Mesos-specific
> clang-tidy checks. We use a docker container for the execution, and
> also provide a script to further streamline the tooling.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh PRE-CREATION 
>   support/mesos-tidy/Dockerfile PRE-CREATION 
>   support/mesos-tidy/README.md PRE-CREATION 
>   support/mesos-tidy/entrypoint.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51525/diff/
> 
> 
> Testing
> ---
> 
> The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
> running that script does produce diagnostics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51553: Changed the way `HAP::updateAllocation()` calls `Resources.apply()`.

[Jiang Yan Xu]

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

(Updated Sept. 1, 2016, 9:35 a.m.)


Review request for mesos and Anindya Sinha.


Repository: mesos


Description (updated)
---

This is needed for supporting LAUNCH operations with shared resources
(More details in https://reviews.apache.org/r/45961/) but this patch
itself has no functional change.

Also, this patch together with https://reviews.apache.org/r/51412/
results in no change in the granularity in which Offer operations are
applied. Each single operation in the ACCEPT call is still individually
applied to Resources objects managed by the master, the allocator and
the sorters, same as before.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 

Diff: https://reviews.apache.org/r/51553/diff/


Testing
---

make check


Thanks,

Jiang Yan Xu

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 1, 2016, 3:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51525/
> ---
> 
> (Updated Sept. 1, 2016, 3:28 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds tooling to both build and execute Mesos-specific
> clang-tidy checks. We use a docker container for the execution, and
> also provide a script to further streamline the tooling.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh PRE-CREATION 
>   support/mesos-tidy/Dockerfile PRE-CREATION 
>   support/mesos-tidy/README.md PRE-CREATION 
>   support/mesos-tidy/entrypoint.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51525/diff/
> 
> 
> Testing
> ---
> 
> The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
> running that script does produce diagnostics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51571: Updated Mesos version in getting started guide.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Sept. 1, 2016, 9:06 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51571/
> ---
> 
> (Updated Sept. 1, 2016, 9:06 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Mesos version in getting started guide.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md ebe52705b8c8757d4a507ce3ae75f56d535a39d1 
> 
> Diff: https://reviews.apache.org/r/51571/diff/
> 
> 
> Testing
> ---
> 
> viewed in website docker container
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Michael Park]

> On Sept. 1, 2016, 11:29 a.m., Michael Park wrote:
> > support/mesos-tidy/entrypoint.sh, line 29
> > 
> >
> > This is my bad.
> > 
> > ```
> > - bear make tests -j $(nproc) || exit 1
> > + bear make -j $(nproc) tests || exit 1
> > ```
> 
> Benjamin Bannier wrote:
> Changed as suggested, not sure what difference it makes though.

Nothing functionally. I just saw that you use `make -j $(nproc) install` below.


- Michael


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


On Sept. 1, 2016, 3:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51525/
> ---
> 
> (Updated Sept. 1, 2016, 3:28 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds tooling to both build and execute Mesos-specific
> clang-tidy checks. We use a docker container for the execution, and
> also provide a script to further streamline the tooling.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh PRE-CREATION 
>   support/mesos-tidy/Dockerfile PRE-CREATION 
>   support/mesos-tidy/README.md PRE-CREATION 
>   support/mesos-tidy/entrypoint.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51525/diff/
> 
> 
> Testing
> ---
> 
> The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
> running that script does produce diagnostics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Benjamin Bannier]

> On Sept. 1, 2016, 1:29 p.m., Michael Park wrote:
> > support/mesos-tidy/Dockerfile, line 29
> > 
> >
> > Can this go right after `MAINTAINER`?

Sure, done.


> On Sept. 1, 2016, 1:29 p.m., Michael Park wrote:
> > support/mesos-tidy/Dockerfile, lines 35-37
> > 
> >
> > Can we try to get rid of this `sslVerify` stuff?
> 
> Michael Park wrote:
> It looks like we can get rid of this by installing the `ca-certificates` 
> package!

That does fix it, thanks for looking that one up.


> On Sept. 1, 2016, 1:29 p.m., Michael Park wrote:
> > support/mesos-tidy/entrypoint.sh, line 26
> > 
> >
> > Do we not need to do
> > 
> > ```
> > CONFIGURE_FLAGS=${CONFIGURE_FLAGS:-''}
> > ```
> > 
> > similar to what we do for `CHECKS` below?
> > 
> > If we don't, it seems to me like we wouldn't need the
> > 
> > ```
> > CHECKS=${CHECKS:-''}
> > ```
> > 
> > either.

You are right, `CHECKS` can also go here.


> On Sept. 1, 2016, 1:29 p.m., Michael Park wrote:
> > support/mesos-tidy/entrypoint.sh, line 29
> > 
> >
> > This is my bad.
> > 
> > ```
> > - bear make tests -j $(nproc) || exit 1
> > + bear make -j $(nproc) tests || exit 1
> > ```

Changed as suggested, not sure what difference it makes though.


- Benjamin


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


On Sept. 1, 2016, 5:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51525/
> ---
> 
> (Updated Sept. 1, 2016, 5:28 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds tooling to both build and execute Mesos-specific
> clang-tidy checks. We use a docker container for the execution, and
> also provide a script to further streamline the tooling.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh PRE-CREATION 
>   support/mesos-tidy/Dockerfile PRE-CREATION 
>   support/mesos-tidy/README.md PRE-CREATION 
>   support/mesos-tidy/entrypoint.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51525/diff/
> 
> 
> Testing
> ---
> 
> The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
> running that script does produce diagnostics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Benjamin Bannier]

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

(Updated Sept. 1, 2016, 5:28 p.m.)


Review request for mesos and Michael Park.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

This commits adds tooling to both build and execute Mesos-specific
clang-tidy checks. We use a docker container for the execution, and
also provide a script to further streamline the tooling.


Diffs (updated)
-

  support/mesos-tidy.sh PRE-CREATION 
  support/mesos-tidy/Dockerfile PRE-CREATION 
  support/mesos-tidy/README.md PRE-CREATION 
  support/mesos-tidy/entrypoint.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/51525/diff/


Testing
---

The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
running that script does produce diagnostics.


Thanks,

Benjamin Bannier

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Michael Park]

> On Sept. 1, 2016, 11:29 a.m., Michael Park wrote:
> > support/mesos-tidy/Dockerfile, lines 35-37
> > 
> >
> > Can we try to get rid of this `sslVerify` stuff?

It looks like we can get rid of this by installing the `ca-certificates` 
package!


- Michael


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


On Sept. 1, 2016, 10:49 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51525/
> ---
> 
> (Updated Sept. 1, 2016, 10:49 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds tooling to both build and execute Mesos-specific
> clang-tidy checks. We use a docker container for the execution, and
> also provide a script to further streamline the tooling.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh PRE-CREATION 
>   support/mesos-tidy/Dockerfile PRE-CREATION 
>   support/mesos-tidy/README.md PRE-CREATION 
>   support/mesos-tidy/entrypoint.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51525/diff/
> 
> 
> Testing
> ---
> 
> The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
> running that script does produce diagnostics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51525]

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 Sept. 1, 2016, 10:49 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51525/
> ---
> 
> (Updated Sept. 1, 2016, 10:49 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds tooling to both build and execute Mesos-specific
> clang-tidy checks. We use a docker container for the execution, and
> also provide a script to further streamline the tooling.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh PRE-CREATION 
>   support/mesos-tidy/Dockerfile PRE-CREATION 
>   support/mesos-tidy/README.md PRE-CREATION 
>   support/mesos-tidy/entrypoint.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51525/diff/
> 
> 
> Testing
> ---
> 
> The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
> running that script does produce diagnostics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Michael Park]

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




support/mesos-tidy.sh (line 22)


```
- CHECKS=${CHECK:-'-*,mesos-*'}
+ CHECKS=${CHECKS:-'-*,mesos-*'}
```



support/mesos-tidy.sh (line 23)


Maybe leave a comment on why we default to `--enable-optimize`



support/mesos-tidy.sh (line 29)


"Please commit or stash any changes before running `mesos-tidy`!."



support/mesos-tidy.sh (lines 34 - 39)


```
docker run \
  --rm \
  -v "${MESOS_DIRECTORY}":/SRC \
  -e CHECKS="${CHECKS}" \
  -e CONFIGURE_FLAGS="${CONFIGURE_FLAGS}" \
  mesosphere/mesos-tidy || exit 1
```



support/mesos-tidy/Dockerfile (line 29)


Can this go right after `MAINTAINER`?



support/mesos-tidy/Dockerfile (lines 35 - 37)


Can we try to get rid of this `sslVerify` stuff?



support/mesos-tidy/Dockerfile (line 49)


Newline after this?



support/mesos-tidy/README.md (line 9)


Backticks around `mesos-tidy`.



support/mesos-tidy/entrypoint.sh (line 26)


Do we not need to do

```
CONFIGURE_FLAGS=${CONFIGURE_FLAGS:-''}
```

similar to what we do for `CHECKS` below?

If we don't, it seems to me like we wouldn't need the

```
CHECKS=${CHECKS:-''}
```

either.



support/mesos-tidy/entrypoint.sh (line 29)


This is my bad.

```
- bear make tests -j $(nproc) || exit 1
+ bear make -j $(nproc) tests || exit 1
```


- Michael Park


On Sept. 1, 2016, 10:49 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51525/
> ---
> 
> (Updated Sept. 1, 2016, 10:49 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds tooling to both build and execute Mesos-specific
> clang-tidy checks. We use a docker container for the execution, and
> also provide a script to further streamline the tooling.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh PRE-CREATION 
>   support/mesos-tidy/Dockerfile PRE-CREATION 
>   support/mesos-tidy/README.md PRE-CREATION 
>   support/mesos-tidy/entrypoint.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51525/diff/
> 
> 
> Testing
> ---
> 
> The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
> running that script does produce diagnostics.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51571: Updated Mesos version in getting started guide.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51571]

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 Sept. 1, 2016, 9:06 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51571/
> ---
> 
> (Updated Sept. 1, 2016, 9:06 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Mesos version in getting started guide.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md ebe52705b8c8757d4a507ce3ae75f56d535a39d1 
> 
> Diff: https://reviews.apache.org/r/51571/diff/
> 
> 
> Testing
> ---
> 
> viewed in website docker container
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Benjamin Bannier]

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

(Updated Sept. 1, 2016, 12:49 p.m.)


Review request for mesos and Michael Park.


Changes
---

Fixup README, be more civil (NO SCREAMING1)


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


Repository: mesos


Description
---

This commits adds tooling to both build and execute Mesos-specific
clang-tidy checks. We use a docker container for the execution, and
also provide a script to further streamline the tooling.


Diffs (updated)
-

  support/mesos-tidy.sh PRE-CREATION 
  support/mesos-tidy/Dockerfile PRE-CREATION 
  support/mesos-tidy/README.md PRE-CREATION 
  support/mesos-tidy/entrypoint.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/51525/diff/


Testing
---

The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
running that script does produce diagnostics.


Thanks,

Benjamin Bannier

Re: Review Request 51525: Added tooling to execute Mesos-specific clang-tidy checks.

[Benjamin Bannier]

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

(Updated Sept. 1, 2016, 12:30 p.m.)


Review request for mesos and Michael Park.


Changes
---

Additional streamlining as discussed offline.


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


Repository: mesos


Description
---

This commits adds tooling to both build and execute Mesos-specific
clang-tidy checks. We use a docker container for the execution, and
also provide a script to further streamline the tooling.


Diffs (updated)
-

  support/mesos-tidy.sh PRE-CREATION 
  support/mesos-tidy/Dockerfile PRE-CREATION 
  support/mesos-tidy/README.md PRE-CREATION 
  support/mesos-tidy/entrypoint.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/51525/diff/


Testing (updated)
---

The image used in `support/mesos-tidy.sh` is available on dockerhub, and 
running that script does produce diagnostics.


Thanks,

Benjamin Bannier

Re: Review Request 51378: Exposed `process::internal::defaultClone` to `process` namespace.

[Alexander Rukletsov]

> On Sept. 1, 2016, 2:05 a.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/posix/subprocess.hpp, lines 55-68
> > 
> >
> > Any reason you need to expose this internal method? Why not just write 
> > your own clone function at the call sites? THis default clone sounds pretty 
> > straight forward to me. Copy that to the call site is probably more 
> > readable.
> > 
> > `process::defaultClone` just sounds like a very weird public interface.
> > 
> > I'd suggest we revert this change.

I'd rather not duplicate code. Copying is not a problem, but since 
currently—read: without child hooks—the only way to add something between 
`fork` and `exec` is to write a custom `clone`, I don't want people to deviate 
and write all kind of `clone`s: based on `fork`, based on `clone`, with 
different stack options... This potentially will lead to hardly debuggable 
issues.

I'd suggest we add (probably Windows-aware) child hooks and _then_ mark this 
function private again.


- Alexander


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


On Aug. 28, 2016, 1:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51378/
> ---
> 
> (Updated Aug. 28, 2016, 1:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón 
> Kleiman, Gilbert Song, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-5961
> https://issues.apache.org/jira/browse/MESOS-5961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed `process::internal::defaultClone` to `process` namespace.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> a871fe484905165eed093124687c4920f3968ccc 
> 
> Diff: https://reviews.apache.org/r/51378/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/51379/
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 51571: Updated Mesos version in getting started guide.

[Joerg Schad]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Updated Mesos version in getting started guide.


Diffs
-

  docs/getting-started.md ebe52705b8c8757d4a507ce3ae75f56d535a39d1 

Diff: https://reviews.apache.org/r/51571/diff/


Testing
---

viewed in website docker container


Thanks,

Joerg Schad