Re: Review Request 50868: Using `OfferedResources` for benchmark test of `SuppressOffers`.

[Guangya Liu]

> On 八月 25, 2016, 10:56 p.m., Jiang Yan Xu wrote:
> > I don't know. Making such distinction feels like splitting hairs to me. 
> > Yeah there are subtle differences between the two concepts but to me:
> > 
> > - `offerCallback` may not be the best name what we could have come up with 
> > even though in `Master::offer()` we do construct offers.
> > - When offer objects are constructed, we call the resources in them 
> > "offered resources". 
> > - When we parse resources from offer objects, we call them "offered 
> > resources".
> > - In this test, namely, the "Allocator" tests, we are indeed only 
> > allocating resources. (I don't see "Offer" objects being created).
> > 
> > Later if the allocator actually hands out offer objects, then we probably 
> > need to change the tests to use offers directly but right now I think 1) 
> > "allocation" is most correct, 2) it's not a big deal if people treat them 
> > as synonymous terms. 
> > 
> > That said, I think reusing the "Allocation" defined at the top of the test 
> > makes sense.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, there are acutally some discussion here 
> https://reviews.apache.org/r/50677/ , the reason that I want to update this 
> is because:
>  
> 1) Make the code consistent, in the allocator test cases, some benchmark 
> test using `OfferedResources` while some using `Allocation`, when people want 
> to add new benchmark test, they will be confused: Which one shall I use, 
> `OfferedResources` or `Allocation`, so here we need to make the code 
> consistent with each other for better reading, easy to maintain and 
> consistent.
> 
> 2) Seems here using `OfferedResources` maybe better, as here we are 
> actually constructing the `offers` on each agent; But the `Allocation` is an 
> allocator concept and it means the resources for a specified framework. Also 
> all of the log messages are highlighting `offer` in benchmark test such as 
> here 
> https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L3885
> 
> 3) The benchmark test is focusing on `offers` even though the allocator 
> know nothing about `offer`, but here we are just simulating `offer 
> operations` in benchmark test. For other unit test cases other than benchmark 
> test, they are focusing on `allocations` for each framework, so it is using 
> `Allocation`. 
> 
> What do you think?
> 
> Jiang Yan Xu wrote:
> Thanks for the reply. I've listed a few critia that I think could explain 
> their subtle differences but I try to see if making such a distinction leads 
> to simplity and clarity of the code. i.e., would everyone agree with the way 
> we make such distinction and would it help readers understand the code 
> better. If it's controversial, then perhaps we should aim for consistently. :)
> 
> In terms of code in this file, yes I agree there's inconsistency and we 
> should address it. To that end I think:
> 
> To 1): The question to me is to choose between *OfferedResources -> 
> Allocation* or *Allocation -> OfferedResources*. I am leaning towards 
> *OfferedResources -> Allocation* and FWIW `Allocation` is a more established 
> usage than `OfferedResources` in this file.
> 
> To 2) and 3): sorry I failed to see the fundamental difference between 
> the benchmarks and the unit tests here in terms of `offers` vs `allocations`. 
> The unit tests, espcially later-added ones, follow basically the same pattern 
> as the benchmarks. The benchmarks are just different in the use of 
> expectations vs. measurements and the number of iterations. It's not 
> convincing to me that in some cases we need to call them OfferedResources and 
> in others we should call them Allocations. 
> 
> So overall I think it's better to change to use the Allocation struct 
> defined at the top to achieve consistency. It looks to me that all 
> occurrences of `offers` in the log messages can be changed to `allocations` 
> as well.
> 
> Let me know what you think.
> 
> Guangya Liu wrote:
> Hi Jiang Yan, are you mentioning the `Allocation` definition here 
> https://github.com/apache/mesos/blob/9c6097f063405279efc07eec22457c2059653f07/src/tests/hierarchical_allocator_tests.cpp#L79-L83
>  ? They are a bit different:
> 
> ```
> struct Allocation
> {
>   FrameworkID frameworkId;
>   hashmap resources;
> };
> ```
> 
> ```
> struct Allocation
> {
>   FrameworkID   frameworkId;
>   SlaveID   slaveId;
>   Resources resources;
> };
> ```
> 
> The above two is a bit difference: The first `Allocation` is an 
> allocation for a framework and the second `Allocation` is actullay an `offer` 
> as one framework can have multiple such `Allocation`.
> 
> So it may not accurate to use `allocations` in the benchmark test as they 
> are actually `offers` for each framework, comments? Thanks.
> 
> Jiang Yan Xu wrote:
>   

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Guangya Liu]

> On 八月 24, 2016, 9:12 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4030
> > 
> >
> > How about adding `ports` resources to the allocation here.
> > 
> > ```
> > // Each agent has a portion of it's resources allocated to a single
> > // framework. We round-robin through the frameworks when allocating.
> > Resources allocation = 
> > Resources::parse("cpus:16;mem:1024;disk:1024").get();
> > 
> > Try<::mesos::Value::Ranges> ranges = fragment(createRange(31000, 
> > 32000), 16);
> > ASSERT_SOME(ranges);
> > ASSERT_EQ(16, ranges->range_size());
> > 
> > allocation += createPorts(ranges.get());
> > ```
> 
> Jacob Janco wrote:
> I took this out to simplify the test.

I think that we should simulate a full allocate scernario in the benchmark test 
to include `ports` as well just like other benchmark test, as we actually have 
some special logic to handle non-scalar resources in sorter when allocate 
resources, comments?


- Guangya


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


On 八月 17, 2016, 2:26 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated 八月 17, 2016, 2:26 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.FrameworkFailover*" make check
> 
> Sample Output:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
> Using 1 agents and 6000 frameworks
> Added 6000 frameworks in 113410us
> Added 1 agents in 6.8398066333mins
> allocator settled after  3.286837mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
>  (609255 ms)[ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
> Using 2 agents and 1 frameworks
> Added 1 frameworks in 190us
> Added 2 agents in 4.752954secs
> allocator settled after  7us
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
>  (6332 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Guangya Liu]

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




src/master/allocator/mesos/hierarchical.cpp (lines 1212 - 1215)


Just nit, I think that we should separate the `_allocate()` and 
`_deallocate()` to use different time elapse. But as the original logic is also 
putting the `_deallocate()` logic in `_allocate()`, so I think that it is OK to 
keep such logic but may need an update if we want to make the time more 
accurate for diffrent actions: `_allocate()` and `_deallocate()`.

```
stopwatch.start();

metrics.allocation_run.start();

_allocate();

metrics.allocation_run.stop();

stopwatch.stop();

VLOG(1) << "Performed allocation for " << allocationCandidates.size()
<< " agents in " << stopwatch.elapsed();

stopwatch.start();

_deallocate();

stopwatch.stop();

VLOG(1) << "Performed deallocation for " << allocationCandidates.size()
<< " agents in " << stopwatch.elapsed();
```


- Guangya Liu


On 九月 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 九月 3, 2016, 6:05 a.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.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> 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 49617: Add benchmark for failover of many frameworks.

[Jacob Janco]

> On Aug. 24, 2016, 9:12 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4030
> > 
> >
> > How about adding `ports` resources to the allocation here.
> > 
> > ```
> > // Each agent has a portion of it's resources allocated to a single
> > // framework. We round-robin through the frameworks when allocating.
> > Resources allocation = 
> > Resources::parse("cpus:16;mem:1024;disk:1024").get();
> > 
> > Try<::mesos::Value::Ranges> ranges = fragment(createRange(31000, 
> > 32000), 16);
> > ASSERT_SOME(ranges);
> > ASSERT_EQ(16, ranges->range_size());
> > 
> > allocation += createPorts(ranges.get());
> > ```

I took this out to simplify the test.


- Jacob


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


On Aug. 17, 2016, 2:26 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated Aug. 17, 2016, 2:26 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.FrameworkFailover*" make check
> 
> Sample Output:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
> Using 1 agents and 6000 frameworks
> Added 6000 frameworks in 113410us
> Added 1 agents in 6.8398066333mins
> allocator settled after  3.286837mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
>  (609255 ms)[ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
> Using 2 agents and 1 frameworks
> Added 1 frameworks in 190us
> Added 2 agents in 4.752954secs
> allocator settled after  7us
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
>  (6332 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Jacob Janco]

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

(Updated Sept. 3, 2016, 6:05 a.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 (updated)
---

- 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.


Diffs (updated)
-

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

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 50693: Added a `createSlaveInfo()` overload that takes a `Resources`.

[Guangya Liu]

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

(Updated 九月 3, 2016, 4:29 a.m.)


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


Summary (updated)
-

Added a `createSlaveInfo()` overload that takes a `Resources`.


Repository: mesos


Description (updated)
---

Also changed the benchmarks to use it because
it's unnecessary to recreate the same Resources
objects for each agent.


Diffs
-

  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 51620: Removed two std::move in MountInfoTable::read.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51620]

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. 3, 2016, 12:25 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51620/
> ---
> 
> (Updated Sept. 3, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-6118
> https://issues.apache.org/jira/browse/MESOS-6118
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since 'entry' is a const reference, the std::move won't reduce the
> extra copying. This patch removed this optimization.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 14ae5a9089916549f691363bc2269e13c5260a14 
> 
> Diff: https://reviews.apache.org/r/51620/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51097: Added a `PortMapper` class.

[Avinash sridharan]

> On Sept. 2, 2016, 11:46 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp,
> >  line 36
> > 
> >
> > Should put before `using std::cout`;

you mean using mesos ... should come before using std:: ...

I think we follow:
using std...

followed by 
using process

followed by 
using mesos

https://github.com/apache/mesos/blob/79b6a9e38bdeb711bd25c890e7ad88a01d7dbfea/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L39


> On Sept. 2, 2016, 11:46 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp,
> >  line 69
> > 
> >
> > I think it would be better to define a `CNIError` exetends from 
> > `Error`, and return `Try, CNIError>` here?

Valid point. Was thinking of something similar since spec::Error was becoming 
unweildy.


> On Sept. 2, 2016, 11:46 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp,
> >  line 69
> > 
> >
> > Should return `Try`, return `"OK"` looks a bit wried.

This is just a place holder function. In follow up patches will be implementing 
execute. Will return "". In this patch. Need to return string representation of 
`spec::NetworkInfo` since that is what is expected.


- Avinash


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


On Sept. 2, 2016, 10:33 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51097/
> ---
> 
> (Updated Sept. 2, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class will embody the logic for implementing the CNI port-mapper
> plugin.
> 
> Also added helper functions in `cni::spec` to be able to log CNI
> plugin error as a JSON formatted string of `spec::error`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> e6967415a689184f13d65a986f13b0af54364fb2 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> 938a9f3bff2a95c8591450f0edafaddb01833e59 
> 
> Diff: https://reviews.apache.org/r/51097/diff/
> 
> 
> Testing
> ---
> 
> Tested the port-mapper with the following CNI config:
> {
> "name": "mynet",
> "type": "port-mapper",
> "chain": "MESOS",
> "delegate": {
>   "type" : "bridge",
>   "bridge": "cni0",
>   "isGateway": true,
>   "ipMasq": true,
>   "ipam": {
>   "type": "host-local",
>   "subnet": "10.22.0.0/16",
>   "routes": [
> { "dst": "0.0.0.0/0" }
>   ]
>   }
> },
> "args" : {
>   "org.apache.mesos" : {
> "network_info" : {
>   "port_mappings": {
> "host_port" : 80,
> "container_port" : 80
>   }
> }
>   }
> }
> }
> 
> and the following environment variables:
> export CNI_COMMAND="ADD"
> export CNI_CONTAINERID="00001110"
> export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
> export CNI_IFNAME="eth0"
> export CNI_NETNS="/etc/netns"
> 
> If we remove fields from the above CNI config, or remove certain environment 
> variables the creation of the `PortMapper` correctly fails. However, if 
> config and environment variables are passed as is it will create the 
> `PortMapper` correctly.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51620: Removed two std::move in MountInfoTable::read.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 2, 2016, 5:25 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51620/
> ---
> 
> (Updated Sept. 2, 2016, 5:25 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-6118
> https://issues.apache.org/jira/browse/MESOS-6118
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since 'entry' is a const reference, the std::move won't reduce the
> extra copying. This patch removed this optimization.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 14ae5a9089916549f691363bc2269e13c5260a14 
> 
> Diff: https://reviews.apache.org/r/51620/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 51620: Removed two std::move in MountInfoTable::read.

[Jie Yu]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

Since 'entry' is a const reference, the std::move won't reduce the
extra copying. This patch removed this optimization.


Diffs
-

  src/linux/fs.cpp 14ae5a9089916549f691363bc2269e13c5260a14 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 51097: Added a `PortMapper` class.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51095, 51096, 51097]

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, 10:33 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51097/
> ---
> 
> (Updated Sept. 2, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class will embody the logic for implementing the CNI port-mapper
> plugin.
> 
> Also added helper functions in `cni::spec` to be able to log CNI
> plugin error as a JSON formatted string of `spec::error`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> e6967415a689184f13d65a986f13b0af54364fb2 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> 938a9f3bff2a95c8591450f0edafaddb01833e59 
> 
> Diff: https://reviews.apache.org/r/51097/diff/
> 
> 
> Testing
> ---
> 
> Tested the port-mapper with the following CNI config:
> {
> "name": "mynet",
> "type": "port-mapper",
> "chain": "MESOS",
> "delegate": {
>   "type" : "bridge",
>   "bridge": "cni0",
>   "isGateway": true,
>   "ipMasq": true,
>   "ipam": {
>   "type": "host-local",
>   "subnet": "10.22.0.0/16",
>   "routes": [
> { "dst": "0.0.0.0/0" }
>   ]
>   }
> },
> "args" : {
>   "org.apache.mesos" : {
> "network_info" : {
>   "port_mappings": {
> "host_port" : 80,
> "container_port" : 80
>   }
> }
>   }
> }
> }
> 
> and the following environment variables:
> export CNI_COMMAND="ADD"
> export CNI_CONTAINERID="00001110"
> export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
> export CNI_IFNAME="eth0"
> export CNI_NETNS="/etc/netns"
> 
> If we remove fields from the above CNI config, or remove certain environment 
> variables the creation of the `PortMapper` correctly fails. However, if 
> config and environment variables are passed as is it will create the 
> `PortMapper` correctly.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51097: Added a `PortMapper` class.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 (line 36)


Should put before `using std::cout`;



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 (line 69)


I think it would be better to define a `CNIError` exetends from `Error`, 
and return `Try, CNIError>` here?



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 (line 69)


Should return `Try`, return `"OK"` looks a bit wried.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 50)


The indent should be 4 spaces.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 187)


Indent should be 4 spaces?


- haosdent huang


On Sept. 2, 2016, 10:33 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51097/
> ---
> 
> (Updated Sept. 2, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class will embody the logic for implementing the CNI port-mapper
> plugin.
> 
> Also added helper functions in `cni::spec` to be able to log CNI
> plugin error as a JSON formatted string of `spec::error`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> e6967415a689184f13d65a986f13b0af54364fb2 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> 938a9f3bff2a95c8591450f0edafaddb01833e59 
> 
> Diff: https://reviews.apache.org/r/51097/diff/
> 
> 
> Testing
> ---
> 
> Tested the port-mapper with the following CNI config:
> {
> "name": "mynet",
> "type": "port-mapper",
> "chain": "MESOS",
> "delegate": {
>   "type" : "bridge",
>   "bridge": "cni0",
>   "isGateway": true,
>   "ipMasq": true,
>   "ipam": {
>   "type": "host-local",
>   "subnet": "10.22.0.0/16",
>   "routes": [
> { "dst": "0.0.0.0/0" }
>   ]
>   }
> },
> "args" : {
>   "org.apache.mesos" : {
> "network_info" : {
>   "port_mappings": {
> "host_port" : 80,
> "container_port" : 80
>   }
> }
>   }
> }
> }
> 
> and the following environment variables:
> export CNI_COMMAND="ADD"
> export CNI_CONTAINERID="00001110"
> export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
> export CNI_IFNAME="eth0"
> export CNI_NETNS="/etc/netns"
> 
> If we remove fields from the above CNI config, or remove certain environment 
> variables the creation of the `PortMapper` correctly fails. However, if 
> config and environment variables are passed as is it will create the 
> `PortMapper` correctly.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 50868: Using `OfferedResources` for benchmark test of `SuppressOffers`.

[Jiang Yan Xu]

> On Aug. 25, 2016, 3:56 p.m., Jiang Yan Xu wrote:
> > I don't know. Making such distinction feels like splitting hairs to me. 
> > Yeah there are subtle differences between the two concepts but to me:
> > 
> > - `offerCallback` may not be the best name what we could have come up with 
> > even though in `Master::offer()` we do construct offers.
> > - When offer objects are constructed, we call the resources in them 
> > "offered resources". 
> > - When we parse resources from offer objects, we call them "offered 
> > resources".
> > - In this test, namely, the "Allocator" tests, we are indeed only 
> > allocating resources. (I don't see "Offer" objects being created).
> > 
> > Later if the allocator actually hands out offer objects, then we probably 
> > need to change the tests to use offers directly but right now I think 1) 
> > "allocation" is most correct, 2) it's not a big deal if people treat them 
> > as synonymous terms. 
> > 
> > That said, I think reusing the "Allocation" defined at the top of the test 
> > makes sense.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, there are acutally some discussion here 
> https://reviews.apache.org/r/50677/ , the reason that I want to update this 
> is because:
>  
> 1) Make the code consistent, in the allocator test cases, some benchmark 
> test using `OfferedResources` while some using `Allocation`, when people want 
> to add new benchmark test, they will be confused: Which one shall I use, 
> `OfferedResources` or `Allocation`, so here we need to make the code 
> consistent with each other for better reading, easy to maintain and 
> consistent.
> 
> 2) Seems here using `OfferedResources` maybe better, as here we are 
> actually constructing the `offers` on each agent; But the `Allocation` is an 
> allocator concept and it means the resources for a specified framework. Also 
> all of the log messages are highlighting `offer` in benchmark test such as 
> here 
> https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L3885
> 
> 3) The benchmark test is focusing on `offers` even though the allocator 
> know nothing about `offer`, but here we are just simulating `offer 
> operations` in benchmark test. For other unit test cases other than benchmark 
> test, they are focusing on `allocations` for each framework, so it is using 
> `Allocation`. 
> 
> What do you think?
> 
> Jiang Yan Xu wrote:
> Thanks for the reply. I've listed a few critia that I think could explain 
> their subtle differences but I try to see if making such a distinction leads 
> to simplity and clarity of the code. i.e., would everyone agree with the way 
> we make such distinction and would it help readers understand the code 
> better. If it's controversial, then perhaps we should aim for consistently. :)
> 
> In terms of code in this file, yes I agree there's inconsistency and we 
> should address it. To that end I think:
> 
> To 1): The question to me is to choose between *OfferedResources -> 
> Allocation* or *Allocation -> OfferedResources*. I am leaning towards 
> *OfferedResources -> Allocation* and FWIW `Allocation` is a more established 
> usage than `OfferedResources` in this file.
> 
> To 2) and 3): sorry I failed to see the fundamental difference between 
> the benchmarks and the unit tests here in terms of `offers` vs `allocations`. 
> The unit tests, espcially later-added ones, follow basically the same pattern 
> as the benchmarks. The benchmarks are just different in the use of 
> expectations vs. measurements and the number of iterations. It's not 
> convincing to me that in some cases we need to call them OfferedResources and 
> in others we should call them Allocations. 
> 
> So overall I think it's better to change to use the Allocation struct 
> defined at the top to achieve consistency. It looks to me that all 
> occurrences of `offers` in the log messages can be changed to `allocations` 
> as well.
> 
> Let me know what you think.
> 
> Guangya Liu wrote:
> Hi Jiang Yan, are you mentioning the `Allocation` definition here 
> https://github.com/apache/mesos/blob/9c6097f063405279efc07eec22457c2059653f07/src/tests/hierarchical_allocator_tests.cpp#L79-L83
>  ? They are a bit different:
> 
> ```
> struct Allocation
> {
>   FrameworkID frameworkId;
>   hashmap resources;
> };
> ```
> 
> ```
> struct Allocation
> {
>   FrameworkID   frameworkId;
>   SlaveID   slaveId;
>   Resources resources;
> };
> ```
> 
> The above two is a bit difference: The first `Allocation` is an 
> allocation for a framework and the second `Allocation` is actullay an `offer` 
> as one framework can have multiple such `Allocation`.
> 
> So it may not accurate to use `allocations` in the benchmark test as they 
> are actually `offers` for each framework, comments? Thanks.

Yes 
[this](https://github.

Re: Review Request 51097: Added a `PortMapper` class.

[Avinash sridharan]

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

(Updated Sept. 2, 2016, 10:33 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

re-aligned the calls returning `Error` with the JSON formatted string of type 
`spec::Error`.


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


Repository: mesos


Description
---

This class will embody the logic for implementing the CNI port-mapper
plugin.

Also added helper functions in `cni::spec` to be able to log CNI
plugin error as a JSON formatted string of `spec::error`.


Diffs (updated)
-

  src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
e6967415a689184f13d65a986f13b0af54364fb2 
  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
938a9f3bff2a95c8591450f0edafaddb01833e59 

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


Testing
---

Tested the port-mapper with the following CNI config:
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS",
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "10.22.0.0/16",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 80,
"container_port" : 80
  }
}
  }
}
}

and the following environment variables:
export CNI_COMMAND="ADD"
export CNI_CONTAINERID="00001110"
export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
export CNI_IFNAME="eth0"
export CNI_NETNS="/etc/netns"

If we remove fields from the above CNI config, or remove certain environment 
variables the creation of the `PortMapper` correctly fails. However, if config 
and environment variables are passed as is it will create the `PortMapper` 
correctly.


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/#review147695
---


Ship it!




Ship It!

- Jiang Yan Xu


On Sept. 2, 2016, 4:22 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50693/
> ---
> 
> (Updated Sept. 2, 2016, 4:22 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 
> 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.

[Jiang Yan Xu]

> On Sept. 1, 2016, 1: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?
> 
> Guangya Liu wrote:
> 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?

To clarify, I am fine with adding this method; I am neutral with changing the 
benchmarks to use it (hence ship it). I am just not sure about the argument 
about the "accuracy" of benchmarks. I have stated my reasoning in the comments 
above. What's you view on this? Could you adjust the review summary?

i.e., I would just say 

```
Summary: Added a `createSlaveInfo()` overload that takes a `Resources`.

Description: Also changed the benchmarks to use it because it's unnecessary to 
recreate the same Resources objects for each agent.
```


- Jiang Yan


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


On Sept. 2, 2016, 4:22 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50693/
> ---
> 
> (Updated Sept. 2, 2016, 4:22 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 
> 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 
> SlaveAndFramew

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

[Jiang Yan Xu]

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


Fix it, then Ship it!




Modulo comments from others and I'll look at the benchmark results before 
merging.


src/master/allocator/mesos/hierarchical.hpp (lines 219 - 223)


There is symmetry between the two so let's name / document them similarly.

```
// Helper for `allocate()` that allocates resources for offers.
void _allocate();

// Helper for `allocate()` that deallocates resources for inverse offers.
void _deallocate();
```



src/master/allocator/mesos/hierarchical.cpp (lines 1555 - 1558)


Given the way the code is structured now, I think we can just move this 
block to `allocate()`, after it calls `_allocate()`.


- Jiang Yan Xu


On Aug. 30, 2016, 9:53 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 30, 2016, 9:53 a.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 50661: Documentation: fix broken links.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50661]

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, 4:35 p.m., Pierre Cheynier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50661/
> ---
> 
> (Updated Sept. 2, 2016, 4:35 p.m.)
> 
> 
> Review request for mesos, Dave Lester, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Remove references to github.com (contains /blob/master..)
> * site Rakefile now handle links contained in HTML table
> 
> The link to github.com in `submitting-a-patch.md` was let on purpose
> as it seems that some users submit PRs by directly editting the file.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 5456f1be5ff1c7abbc7efd4f6e4130f49b1d6f90 
>   docs/docker-volume.md 2dbd0441a924c9360c1d6432bcc36febe925268b 
>   docs/working-groups.md e9e1076ae809d4a7e198d7eb7a612691912f20c3 
>   site/Rakefile 39aeb3cfec2a419000fb8c9625b79683eff92bdc 
> 
> Diff: https://reviews.apache.org/r/50661/diff/
> 
> 
> Testing
> ---
> 
> Check that doc links still work both in github.com and on the website by:
> * Using the `mesos/website` container
> * Using a Markdown editor
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 51609: Updated formatting in HealthChecker for consistency.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Sept. 2, 2016, 4:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51609/
> ---
> 
> (Updated Sept. 2, 2016, 4:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> f373df19fc8af8e9650be61e3b101e89362a67cd 
> 
> Diff: https://reviews.apache.org/r/51609/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51608: Extracted "curl" binary into HTTP_CHECK_COMMAND constant.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Sept. 2, 2016, 4:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51608/
> ---
> 
> (Updated Sept. 2, 2016, 4:44 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Bugs: MESOS-6117
> https://issues.apache.org/jira/browse/MESOS-6117
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> f373df19fc8af8e9650be61e3b101e89362a67cd 
> 
> Diff: https://reviews.apache.org/r/51608/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51605: Added "mesos-tcp-connect" binary.

[haosdent huang]

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




src/CMakeLists.txt (lines 518 - 524)


I think should put after 
```
add_subdirectory(docker/)
```



src/Makefile.am (line 1377)


I think the name `mesos-tcp-connect` may be not clear enough. Should we add 
something like `health-check` into its name to indicate it is used for health 
check?


- haosdent huang


On Sept. 2, 2016, 4:42 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51605/
> ---
> 
> (Updated Sept. 2, 2016, 4:42 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
> and haosdent huang.
> 
> 
> Bugs: MESOS-6119
> https://issues.apache.org/jira/browse/MESOS-6119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To remove dependency on `bash` for TCP health checks, introduce
> a separate light-weight binary (without libmesos dependency) for
> probing TCP connections.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt b14132abf49a73defd3e4774330b5227726b9207 
>   src/Makefile.am 96edf5b8a4135355ee412e5837e86165885975f8 
>   src/health-check/CMakeLists.txt PRE-CREATION 
>   src/health-check/tcp_connect.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51605/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/51607/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 51608: Extracted "curl" binary into HTTP_CHECK_COMMAND constant.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and haosdent huang.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/health-check/health_checker.cpp f373df19fc8af8e9650be61e3b101e89362a67cd 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Review Request 51609: Updated formatting in HealthChecker for consistency.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and haosdent huang.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/health-check/health_checker.cpp f373df19fc8af8e9650be61e3b101e89362a67cd 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Review Request 51607: Used mesos-tcp-connect binary in TCP health checks.

[Alexander Rukletsov]

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

Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
and haosdent huang.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/docker/executor.cpp f5981b080c0b81ef7025527ab26b36d7a01f009b 
  src/health-check/health_checker.hpp 392b4d5bd1e5831994b9366c1eb5a2911e19860f 
  src/health-check/health_checker.cpp f373df19fc8af8e9650be61e3b101e89362a67cd 
  src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 

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


Testing
---

make check on Mac OS 10.11.6


Thanks,

Alexander Rukletsov

Review Request 51605: Added "mesos-tcp-connect" binary.

[Alexander Rukletsov]

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

Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
and haosdent huang.


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


Repository: mesos


Description
---

To remove dependency on `bash` for TCP health checks, introduce
a separate light-weight binary (without libmesos dependency) for
probing TCP connections.


Diffs
-

  src/CMakeLists.txt b14132abf49a73defd3e4774330b5227726b9207 
  src/Makefile.am 96edf5b8a4135355ee412e5837e86165885975f8 
  src/health-check/CMakeLists.txt PRE-CREATION 
  src/health-check/tcp_connect.cpp PRE-CREATION 

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


Testing
---

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


Thanks,

Alexander Rukletsov

Review Request 51606: Libprocess: Added target for "mesos-tcp-connect" binary.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and haosdent huang.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
f7cba60c9ef7c7ed27819dcf4939c0c51f80d49e 

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


Testing
---

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


Thanks,

Alexander Rukletsov

Re: Review Request 50661: Documentation: fix broken links.

[Pierre Cheynier]

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

(Updated Sept. 2, 2016, 4:35 p.m.)


Review request for mesos, Dave Lester, haosdent huang, and Vinod Kone.


Changes
---

Fix some indentation in HTML table (remarks done by haosdent).


Repository: mesos


Description
---

* Remove references to github.com (contains /blob/master..)
* site Rakefile now handle links contained in HTML table

The link to github.com in `submitting-a-patch.md` was let on purpose
as it seems that some users submit PRs by directly editting the file.


Diffs (updated)
-

  docs/authorization.md 5456f1be5ff1c7abbc7efd4f6e4130f49b1d6f90 
  docs/docker-volume.md 2dbd0441a924c9360c1d6432bcc36febe925268b 
  docs/working-groups.md e9e1076ae809d4a7e198d7eb7a612691912f20c3 
  site/Rakefile 39aeb3cfec2a419000fb8c9625b79683eff92bdc 

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


Testing
---

Check that doc links still work both in github.com and on the website by:
* Using the `mesos/website` container
* Using a Markdown editor


Thanks,

Pierre Cheynier

Re: Review Request 51603: Fixed remaining quoting issues.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51603]

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, 3:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51603/
> ---
> 
> (Updated Sept. 2, 2016, 3:17 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `CONFIGURE_FLAGS` might be multiple arguments so a passed string
> should always be exploded when used. Conversely, `SRCDIR` can only be
> used as a single argument; we do know that it does not contain spaces,
> but should quote it nevertheless for consistency.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh 78198d900556427091ab08f25b6df4fc9428dd93 
> 
> Diff: https://reviews.apache.org/r/51603/diff/
> 
> 
> Testing
> ---
> 
> Passing `--enable-libevent --enable-ssl` to `support/mesos-tidy.sh` fails 
> before this patch, and succeeds after.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51603: Fixed remaining quoting issues.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 2, 2016, 3:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51603/
> ---
> 
> (Updated Sept. 2, 2016, 3:17 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `CONFIGURE_FLAGS` might be multiple arguments so a passed string
> should always be exploded when used. Conversely, `SRCDIR` can only be
> used as a single argument; we do know that it does not contain spaces,
> but should quote it nevertheless for consistency.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh 78198d900556427091ab08f25b6df4fc9428dd93 
> 
> Diff: https://reviews.apache.org/r/51603/diff/
> 
> 
> Testing
> ---
> 
> Passing `--enable-libevent --enable-ssl` to `support/mesos-tidy.sh` fails 
> before this patch, and succeeds after.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 51603: Fixed remaining quoting issues.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

`CONFIGURE_FLAGS` might be multiple arguments so a passed string
should always be exploded when used. Conversely, `SRCDIR` can only be
used as a single argument; we do know that it does not contain spaces,
but should quote it nevertheless for consistency.


Diffs
-

  support/mesos-tidy/entrypoint.sh 78198d900556427091ab08f25b6df4fc9428dd93 

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


Testing
---

Passing `--enable-libevent --enable-ssl` to `support/mesos-tidy.sh` fails 
before this patch, and succeeds after.


Thanks,

Benjamin Bannier

Re: Review Request 51359: Added unit test for provisioner helper findContainerDir.

[Guangya Liu]

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




src/tests/containerizer/provisioner_paths_tests.cpp (lines 56 - 61)


How about

```
ASSERT_SOME(os::mkdir(containerDir1));
ASSERT_SOME(os::mkdir(containerDir2));
ASSERT_SOME(os::mkdir(containerDir3));
```



src/tests/containerizer/provisioner_paths_tests.cpp (lines 63 - 65)


How about

```
EXPECT_SOME_EQ(
containerDir1,
findContainerDir(provisionerDir, child1).get());
```

and ditto for others


- Guangya Liu


On 八月 29, 2016, 9:20 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51359/
> ---
> 
> (Updated 八月 29, 2016, 9:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for provisioner helper findContainerDir.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8dc4175c60e4a9776ddb8ad21774fa4b30c28d00 
>   src/tests/containerizer/provisioner_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51359/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51359: Added unit test for provisioner helper findContainerDir.

[Guangya Liu]

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




src/tests/containerizer/provisioner_paths_tests.cpp (line 39)


move this right before #52 where it was used first?


- Guangya Liu


On 八月 29, 2016, 9:20 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51359/
> ---
> 
> (Updated 八月 29, 2016, 9:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for provisioner helper findContainerDir.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8dc4175c60e4a9776ddb8ad21774fa4b30c28d00 
>   src/tests/containerizer/provisioner_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51359/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51358: Implemented recursive helper method findContainerDir for provisioner.

[Guangya Liu]

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




src/slave/containerizer/mesos/provisioner/paths.cpp (line 111)


kill this



src/slave/containerizer/mesos/provisioner/paths.cpp (lines 132 - 133)


How about putting the `container directory path` into the error message?


- Guangya Liu


On 八月 29, 2016, 9:20 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51358/
> ---
> 
> (Updated 八月 29, 2016, 9:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented recursive helper method findContainerDir for provisioner.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/paths.hpp 
> 9829d6b52c8547ae22297a5bc47852ce5a219e4c 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 86a45f30c22dc1e41f4779c0ce8c11d02dcc46bb 
> 
> Diff: https://reviews.apache.org/r/51358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

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

[Guangya Liu]

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




src/master/allocator/mesos/hierarchical.cpp (lines 686 - 689)


Since here you are applying the `operations` one by one, I think that it is 
better to update the log message here by adding the `operation` in the output 
here so that people will know which `operation` updated the resources each time.


- Guangya Liu


On 九月 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 九月 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.

[Guangya Liu]

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



Jacob, can you please help rebase your patch due to some patches related to 
`shared resources` are now merged in `hierarchical.cpp`? Thanks.

- Guangya Liu


On 八月 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 八月 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 51257: Add external process container logger.

[Will Rouesnel]

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

(Updated Sept. 2, 2016, 11:26 a.m.)


Review request for mesos and Joseph Wu.


Changes
---

Fix outstanding issues.


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


Repository: mesos


Description (updated)
---

Adds the external process container logger. This functions like the
logrotate container logger, but instead calls a custom host binary
(or script) and passes the executorInfo as JSON via environment
variables. This makes it very easy for users to configure custom
logging solutions, without needing to write and maintain logging
modules.

Tests passing and complete.


Diffs (updated)
-

  src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
  src/slave/container_loggers/lib_externallogger.hpp PRE-CREATION 
  src/slave/container_loggers/lib_externallogger.cpp PRE-CREATION 
  src/tests/container_logger_external.sh PRE-CREATION 
  src/tests/container_logger_tests.cpp e8f934106510fe02b8b92be19c918a1e5c0b78fd 
  src/tests/module.hpp e661d95fa44fc1aedfe83c564c826d5b7d32c85b 
  src/tests/module.cpp 5b83fd6358ddea4c9d849b8992e1a6040ef74505 

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


Testing
---

Adds ContainerLoggerTest.EXTERNAL_RecieveEnvironment which tests all major 
parameters of the change.

A synthetic external container logger is provided by the script 
tests/container_logger_external.sh which is setup to fail if any important 
output is unavailable to the logging process. 

The other basic checks are duplicated from the Logrotate container logger, from 
where this change inherits a lot of its code.


Thanks,

Will Rouesnel

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, 11:22 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 51476: Made the `TaskInfo` argument in `launchExecutor()` optional.

[Anand Mazumdar]

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

(Updated Sept. 2, 2016, 9:28 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Added an explicit conditional check to not pass `taskInfo` for custom executors.


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


Repository: mesos


Description
---

Launching a task group requires just the `ExecutorInfo`. We had
already refactored the containerizer `launch()` interface to make
`TaskInfo` as an optional. This change funnels it through.


Diffs (updated)
-

  src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
  src/slave/slave.cpp 5d162d00a8a9a0e318c39bda93dd584f23c87987 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51594: Properly quoted string in shell script.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 2, 2016, 8:09 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51594/
> ---
> 
> (Updated Sept. 2, 2016, 8:09 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-4907
> https://issues.apache.org/jira/browse/MESOS-4907
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was intended to be formatted as code in markdown style, instead
> it was interpreted as a command to run. Use single quotes to prevent
> interpretion of the string.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh 16d436381f4c3fdd716903e92e0e395de4335238 
> 
> Diff: https://reviews.apache.org/r/51594/diff/
> 
> 
> Testing
> ---
> 
> With an uncommitted change in the source tree, 
> 
> * before:
> ./support/mesos-tidy.sh: line 33: mesos-tidy: command not found
> Please commit or stash any changes before running .
> 
> * after:
> Please commit or stash any changes before running `mesos-tidy`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 51594: Properly quoted string in shell script.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

This was intended to be formatted as code in markdown style, instead
it was interpreted as a command to run. Use single quotes to prevent
interpretion of the string.


Diffs
-

  support/mesos-tidy.sh 16d436381f4c3fdd716903e92e0e395de4335238 

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


Testing
---

With an uncommitted change in the source tree, 

* before:
./support/mesos-tidy.sh: line 33: mesos-tidy: command not found
Please commit or stash any changes before running .

* after:
Please commit or stash any changes before running `mesos-tidy`.


Thanks,

Benjamin Bannier

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

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 1, 2016, 6:11 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51592/
> ---
> 
> (Updated Sept. 1, 2016, 6:11 p.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 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Qian Zhang]

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




src/slave/slave.hpp (line 908)


Here the key of this map is all the task IDs of the `TaskGroupInfo`, but 
actually `TaskGroupInfo` already have such info in it (all its tasks), so it 
seems a bit duplicated, maybe `hashset queuedTaskGroups` should 
be enough?


- Qian Zhang


On Aug. 28, 2016, 1:44 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51477/
> ---
> 
> (Updated Aug. 28, 2016, 1:44 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes implements the `runTaskGroup()` handler on the
> agent ensuring that task group is sent atomically to the executor
> via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
> to go through a common handler function. Also, it ensures that all
> tasks in `framework->pending`/`queuedTasks` that are killed before
> running the task group result in all the tasks being killed.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
>   src/slave/slave.cpp 5d162d00a8a9a0e318c39bda93dd584f23c87987 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>