Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

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

(Updated Sept. 6, 2018, 8:29 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón Kleiman.


Changes
---

Posted sorter benchmark results.


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


Repository: mesos


Description
---

This patch lets `class Resources` only store shared
pointers to the underlying resource objects, so that
read-only filter operations such as `reserved()`,
`unreserved()` and etc. can avoid making copies of
the whole resource objects. Instead, only shared pointers
are copied.

In write operations, we check if there are more than one
references to the resource object. If so, a copy is made
for safe mutation without affecting owners.

To maintain the usage abstraction that `class Resources`
still holds resource objects, we utilize
`boost::indirect_iterator` iterator adapter to deference
the shared pointers as we iterate.


Diffs
-

  include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
  include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
  src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
  src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 


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


Testing (updated)
---

make check

Benchmarked on a Ubuntu box using clang -O2 optimized build with an i5 
processor, frequency fixed at 1.6GHz.

## Resources benchmarks (Measure geo-mean of various operations):

Normalized Geomean latency

Master  Copy-on-write
Arithmetic  1   1.09
Filter  1   0.28
Contains1   0.27
Parse   1   1.01

## Sample allocator benchmark (Measure first allocation latency):

HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1 (1k agents, 50 frameworks):

Master 1
Copy-on-write 0.72

HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1 (1k agents, 50 
frameworks):

Master 1
Copy-on-write 0.72

## Sorter benchmark

### BENCHMARK_FullSort

Master:

Using 3 agents and 500 clients
Added 500 clients in 2.261373ms
Added 3 agents in 322.252441ms
Added allocations for 3 agents in 823.631826ms
Full sort of 500 clients took 8662ns
No-op sort of 500 clients took 809ns
Removed allocations for 3 agents in 8.482659084secs
Removed 3 agents in 417.988004ms
Removed 500 clients in 17.855575ms


Copy-on-write:

Using 3 agents and 500 clients (**normalized to the master above**)
Added 500 clients in 2.464647ms (**1.09**)
Added 3 agents in 205.071423ms (**0.64**)
Added allocations for 3 agents in 156.340883ms (**0.19**)
Full sort of 500 clients took 1007ns (**0.12**)
No-op sort of 500 clients took 204ns (**0.02**)
Removed allocations for 3 agents in 7.215175016secs (**0.85**)
Removed 3 agents in 304.998116ms (**0.73**)
Removed 500 clients in 3.195079ms (**0.18**)


### BENCHMARK_HierarchyFullSort

Master:

Added 1056 clients in 36.571505ms
Added 1 agents in 114.165111ms
Added allocations for 1 agents in 569.144686ms
Full sort of 1056 clients took 43485ns
No-op sort of 1056 clients took 676ns
Removed allocations for 1 agents in 5.656517904secs
Removed 1 agents in 153.016733ms
Removed 1056 clients in 7.169549ms

Copy-on-write:

Added 1056 clients in 36.468822ms (**0.997**)
Added 1 agents in 67.733365ms (**0.59**)
Added allocations for 1 agents in 99.695723ms (**0.175**)
Full sort of 1056 clients took 29729ns (**0.68**)
No-op sort of 1056 clients took 704ns (**1.04**)
Removed allocations for 1 agents in 4.824389788secs (**0.85**)
Removed 1 agents in 101.319295ms (**0.66**)
Removed 1056 clients in 2.480967ms (**0.35**)


Thanks,

Meng Zhu

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

> On Sept. 6, 2018, 3:27 a.m., Benjamin Bannier wrote:
> > include/mesos/resources.hpp
> > Line 588 (original), 590 (patched)
> > 
> >
> > `s/Resource/Resource_/`?

`Resource_` is not exposed outside, it is implicitly converted to `Resource`. 
Will comment on this.


> On Sept. 6, 2018, 3:27 a.m., Benjamin Bannier wrote:
> > src/v1/resources.cpp
> > Lines 2117-2118 (original), 2154-2178 (patched)
> > 
> >
> > This function looks redundant and identical to `Resources::add(const 
> > Resource&)` to me. It also introduces another place where we leak 
> > implementation details into the interface.
> > 
> > Suggest to remove and instead let callers perform the deref. We could 
> > always add something similar back once we want to support `move`'ing a 
> > `that`.
> 
> Benjamin Mahler wrote:
> I think Meng forgot to publish a reply here, but you can see the reply on 
> my similar comment above. If we don't have this and the caller has to de-ref, 
> then we have to copy the resource object, whereas this interface allows us to 
> copy the shared_ptr.
> 
> Also note that this is not part of the public interface, it's private.

The difference lies in:

```
 // Cannot be combined with any existing Resource object.
  if (!found) {
resources.push_back(that);
  }
```

I will copy my earlier reply to BenM:

Consider adding two non-mergeble Resources(e.g. 1 cpu + 1 mem), if we only have 
Resources::add(const Resource_& that), we will always need to 
make_shared(Resource_). If we directly pass shared_ptr, we can then just 
push_back. For any non-mergable Resource_, we will be able to save one 
make_shared.


One rule of thumb I figured is that, we should avoid make_shared when possible 
(because it makes a copy). This means if we already have a shared_ptr we should 
just use it and pass it around. This also explains why I use shared_ptr for 
foreach loops.


- Meng


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


On Sept. 6, 2018, 2:56 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68490/
> ---
> 
> (Updated Sept. 6, 2018, 2:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón 
> Kleiman.
> 
> 
> Bugs: MESOS-6765
> https://issues.apache.org/jira/browse/MESOS-6765
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch lets `class Resources` only store shared
> pointers to the underlying resource objects, so that
> read-only filter operations such as `reserved()`,
> `unreserved()` and etc. can avoid making copies of
> the whole resource objects. Instead, only shared pointers
> are copied.
> 
> In write operations, we check if there are more than one
> references to the resource object. If so, a copy is made
> for safe mutation without affecting owners.
> 
> To maintain the usage abstraction that `class Resources`
> still holds resource objects, we utilize
> `boost::indirect_iterator` iterator adapter to deference
> the shared pointers as we iterate.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
>   include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
>   src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
>   src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 
> 
> 
> Diff: https://reviews.apache.org/r/68490/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarked on a Ubuntu box using clang -O2 optimized build with an i5 
> processor, frequency fixed at 1.6GHz.
> 
> Resources benchmarks (Measure geo-mean of various operations):
> 
> Normalized Geomean latency
>   
>   Master  Copy-on-write
> Arithmetic1   1.09
> Filter1   0.28
> Contains  1   0.27
> Parse 1   1.01
> 
> Sample allocator benchmark (Measure first allocation latency):
> 
> HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1 (1k agents, 50 
> frameworks):
> 
> Master 1
> Copy-on-write 0.72
> 
> HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1 (1k agents, 50 
> frameworks):
> 
> Master 1
> Copy-on-write 0.72
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68639: Renamed `Resources::resources` vector and typedef its item type.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68639 was successfully built and tested.

Reviews applied: `['68490', '68639']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2307/mesos-review-68639

- Mesos Reviewbot Windows


On Sept. 7, 2018, 12:35 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68639/
> ---
> 
> (Updated Sept. 7, 2018, 12:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón 
> Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to the copy-on-write optimization (MESOS-6765), one needs to
> check the `use_count` of `Resource_` before mutating. Currently,
> there is no mechanism to enforce this. As a short-term mitigation
> measure, we rename `resources` to
> `resourcesNoMutationWithoutExclusiveOwnership` and typedef its item
> type to `Resource_UnSafe`
> to alert people about obtaining an exclusive ownership before mutating
> the `Resource_` objects.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
>   include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
>   src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
>   src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 
> 
> 
> Diff: https://reviews.apache.org/r/68639/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Benjamin Mahler]

> On Sept. 6, 2018, 10:27 a.m., Benjamin Bannier wrote:
> > src/v1/resources.cpp
> > Lines 2117-2118 (original), 2154-2178 (patched)
> > 
> >
> > This function looks redundant and identical to `Resources::add(const 
> > Resource&)` to me. It also introduces another place where we leak 
> > implementation details into the interface.
> > 
> > Suggest to remove and instead let callers perform the deref. We could 
> > always add something similar back once we want to support `move`'ing a 
> > `that`.

I think Meng forgot to publish a reply here, but you can see the reply on my 
similar comment above. If we don't have this and the caller has to de-ref, then 
we have to copy the resource object, whereas this interface allows us to copy 
the shared_ptr.

Also note that this is not part of the public interface, it's private.


- Benjamin


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


On Sept. 6, 2018, 9:56 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68490/
> ---
> 
> (Updated Sept. 6, 2018, 9:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón 
> Kleiman.
> 
> 
> Bugs: MESOS-6765
> https://issues.apache.org/jira/browse/MESOS-6765
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch lets `class Resources` only store shared
> pointers to the underlying resource objects, so that
> read-only filter operations such as `reserved()`,
> `unreserved()` and etc. can avoid making copies of
> the whole resource objects. Instead, only shared pointers
> are copied.
> 
> In write operations, we check if there are more than one
> references to the resource object. If so, a copy is made
> for safe mutation without affecting owners.
> 
> To maintain the usage abstraction that `class Resources`
> still holds resource objects, we utilize
> `boost::indirect_iterator` iterator adapter to deference
> the shared pointers as we iterate.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
>   include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
>   src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
>   src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 
> 
> 
> Diff: https://reviews.apache.org/r/68490/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarked on a Ubuntu box using clang -O2 optimized build with an i5 
> processor, frequency fixed at 1.6GHz.
> 
> Resources benchmarks (Measure geo-mean of various operations):
> 
> Normalized Geomean latency
>   
>   Master  Copy-on-write
> Arithmetic1   1.09
> Filter1   0.28
> Contains  1   0.27
> Parse 1   1.01
> 
> Sample allocator benchmark (Measure first allocation latency):
> 
> HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1 (1k agents, 50 
> frameworks):
> 
> Master 1
> Copy-on-write 0.72
> 
> HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1 (1k agents, 50 
> frameworks):
> 
> Master 1
> Copy-on-write 0.72
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68134: Avoided reviving on behalf of scheduler after agent reconfiguration.

[Greg Mann]

> On Aug. 6, 2018, 9:49 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 759-768 (original), 757-759 (patched)
> > 
> >
> > Probably an explanation to the reader is warranted on why we do this?
> 
> Greg Mann wrote:
> I was thinking that a more suitable place for such a comment would be the 
> callsite of `removeFilters()`, and it turns out we do already have one there: 
> https://github.com/apache/mesos/blob/7ad23b87626c215703118c74602f53ce2d12cc59/src/master/allocator/mesos/hierarchical.cpp#L655-L663
> 
> I'm going to drop this issue for now, LMK if you think the linked comment 
> is insufficient.

Also, w.r.t. testing: I do not believe we have coverage of the filter removal. 
I'll look into this.


- Greg


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


On July 31, 2018, 6:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68134/
> ---
> 
> (Updated July 31, 2018, 6:57 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9124
> https://issues.apache.org/jira/browse/MESOS-9124
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When agent reconfiguration was enabled in Mesos, the allocator was
> also updated to remove all offer filters associated with an agent when
> that agent's attributes change. In addition, whenever filters for an
> agent are removed, the framework is revived for all roles that it is
> registered in.
> 
> While this ensures that schedulers will have an opportunity to use
> resources on an agent after reconfiguration, modifying the scheduler's
> suppression may put the scheduler in an inconsistent state, where it
> believes it is suppressed in a particular role when it is not.
> 
> This patch eliminates the suppression modification code, while
> keeping the code which removes a reconfigured agent's offer filters.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 35992474eacb8b14ae57e1dc23307e1542f63cb5 
> 
> 
> Diff: https://reviews.apache.org/r/68134/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 68639: Renamed `Resources::resources` vector and typedef its item type.

[Meng Zhu]

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

(Updated Sept. 6, 2018, 5:35 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón Kleiman.


Changes
---

Addressed Benjamin's comment.


Summary (updated)
-

Renamed `Resources::resources` vector and typedef its item type.


Repository: mesos


Description (updated)
---

Due to the copy-on-write optimization (MESOS-6765), one needs to
check the `use_count` of `Resource_` before mutating. Currently,
there is no mechanism to enforce this. As a short-term mitigation
measure, we rename `resources` to
`resourcesNoMutationWithoutExclusiveOwnership` and typedef its item
type to `Resource_UnSafe`
to alert people about obtaining an exclusive ownership before mutating
the `Resource_` objects.


Diffs (updated)
-

  include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
  include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
  src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
  src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 68134: Avoided reviving on behalf of scheduler after agent reconfiguration.

[Greg Mann]

> On Aug. 6, 2018, 9:49 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 759-768 (original), 757-759 (patched)
> > 
> >
> > Probably an explanation to the reader is warranted on why we do this?

I was thinking that a more suitable place for such a comment would be the 
callsite of `removeFilters()`, and it turns out we do already have one there: 
https://github.com/apache/mesos/blob/7ad23b87626c215703118c74602f53ce2d12cc59/src/master/allocator/mesos/hierarchical.cpp#L655-L663

I'm going to drop this issue for now, LMK if you think the linked comment is 
insufficient.


- Greg


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


On July 31, 2018, 6:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68134/
> ---
> 
> (Updated July 31, 2018, 6:57 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9124
> https://issues.apache.org/jira/browse/MESOS-9124
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When agent reconfiguration was enabled in Mesos, the allocator was
> also updated to remove all offer filters associated with an agent when
> that agent's attributes change. In addition, whenever filters for an
> agent are removed, the framework is revived for all roles that it is
> registered in.
> 
> While this ensures that schedulers will have an opportunity to use
> resources on an agent after reconfiguration, modifying the scheduler's
> suppression may put the scheduler in an inconsistent state, where it
> believes it is suppressed in a particular role when it is not.
> 
> This patch eliminates the suppression modification code, while
> keeping the code which removes a reconfigured agent's offer filters.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 35992474eacb8b14ae57e1dc23307e1542f63cb5 
> 
> 
> Diff: https://reviews.apache.org/r/68134/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 68662: Avoided double copying of outgoing messages in the master.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68662 was successfully built and tested.

Reviews applied: `['68662']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2306/mesos-review-68662

- Mesos Reviewbot Windows


On Sept. 6, 2018, 3:39 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68662/
> ---
> 
> (Updated Sept. 6, 2018, 3:39 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Greg Mann.
> 
> 
> Bugs: MESOS-9213
> https://issues.apache.org/jira/browse/MESOS-9213
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When framework metrics were introduced, there was an evolve + devolve
> conversion occuring in order to map old style messages to the new
> `scheduler::Event::Type`. This results in copying all outgoing
> messages twice.
> 
> This patch avoids the copying by adding overloads for the old style
> messages.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ec2ae01ec813ff7fcb0e2a3d0dbad3c3f9993a98 
>   src/master/metrics.hpp 14f894a40e96be93037e0d262ddc4b04a6fcc301 
>   src/master/metrics.cpp 655c75a042a3bfeb264a55d7495f957c407a4d6d 
> 
> 
> Diff: https://reviews.apache.org/r/68662/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68662: Avoided double copying of outgoing messages in the master.

[Benjamin Mahler]

> On Sept. 7, 2018, midnight, Gastón Kleiman wrote:
> > src/master/metrics.hpp
> > Lines 243 (patched)
> > 
> >
> > This kind message is not sent to v0 schedulers, so we should take it 
> > out.

Chatted with Gastón offline, we weren't able to take this out because it's a 
run-time invariant. We could remove this if we update the master code to avoid 
calling it by checking its invariants and upgrading into an Event.


- Benjamin


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


On Sept. 6, 2018, 10:39 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68662/
> ---
> 
> (Updated Sept. 6, 2018, 10:39 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Greg Mann.
> 
> 
> Bugs: MESOS-9213
> https://issues.apache.org/jira/browse/MESOS-9213
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When framework metrics were introduced, there was an evolve + devolve
> conversion occuring in order to map old style messages to the new
> `scheduler::Event::Type`. This results in copying all outgoing
> messages twice.
> 
> This patch avoids the copying by adding overloads for the old style
> messages.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ec2ae01ec813ff7fcb0e2a3d0dbad3c3f9993a98 
>   src/master/metrics.hpp 14f894a40e96be93037e0d262ddc4b04a6fcc301 
>   src/master/metrics.cpp 655c75a042a3bfeb264a55d7495f957c407a4d6d 
> 
> 
> Diff: https://reviews.apache.org/r/68662/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68662: Avoided double copying of outgoing messages in the master.

[Greg Mann]

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


Fix it, then Ship it!





src/master/metrics.hpp
Lines 231 (patched)


Nit: this comment sounds a bit awkward to me? Maybe something like:

"Overloads to convert old, unversioned scheduler messages into their event 
counterparts."


- Greg Mann


On Sept. 6, 2018, 10:39 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68662/
> ---
> 
> (Updated Sept. 6, 2018, 10:39 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Greg Mann.
> 
> 
> Bugs: MESOS-9213
> https://issues.apache.org/jira/browse/MESOS-9213
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When framework metrics were introduced, there was an evolve + devolve
> conversion occuring in order to map old style messages to the new
> `scheduler::Event::Type`. This results in copying all outgoing
> messages twice.
> 
> This patch avoids the copying by adding overloads for the old style
> messages.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ec2ae01ec813ff7fcb0e2a3d0dbad3c3f9993a98 
>   src/master/metrics.hpp 14f894a40e96be93037e0d262ddc4b04a6fcc301 
>   src/master/metrics.cpp 655c75a042a3bfeb264a55d7495f957c407a4d6d 
> 
> 
> Diff: https://reviews.apache.org/r/68662/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68662: Avoided double copying of outgoing messages in the master.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/master/metrics.hpp
Lines 243 (patched)


This kind message is not sent to v0 schedulers, so we should take it out.


- Gastón Kleiman


On Sept. 6, 2018, 3:39 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68662/
> ---
> 
> (Updated Sept. 6, 2018, 3:39 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Greg Mann.
> 
> 
> Bugs: MESOS-9213
> https://issues.apache.org/jira/browse/MESOS-9213
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When framework metrics were introduced, there was an evolve + devolve
> conversion occuring in order to map old style messages to the new
> `scheduler::Event::Type`. This results in copying all outgoing
> messages twice.
> 
> This patch avoids the copying by adding overloads for the old style
> messages.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ec2ae01ec813ff7fcb0e2a3d0dbad3c3f9993a98 
>   src/master/metrics.hpp 14f894a40e96be93037e0d262ddc4b04a6fcc301 
>   src/master/metrics.cpp 655c75a042a3bfeb264a55d7495f957c407a4d6d 
> 
> 
> Diff: https://reviews.apache.org/r/68662/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68637: Avoided extra copying of Resources in Master::offer.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['68637']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2305/mesos-review-68637

Relevant logs:

- 
[mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2305/mesos-review-68637/logs/mesos-tests.log):

```
I0906 23:05:41.102778 10580 executor.cpp:796] Shutting down
I0906 23:05:41.102778 10580 executor.cpp:909] Sending SIGTERM to process tree 
at pid 78200 at executor(1)@192.10.1.5:62253
I0906 23:05:41.102778  9260 slave.cpp:909] Agent terminating
W0906 23:05:41.102778  9260 slave.cpp:3917] Ignoring shutdown framework 
59aa0497-24a8-4770-be6a-c309f80fbc07- because it is terminating
I0906 23:05:41.102778 13420 master.cpp:11030] Removing task 
61fadfb7-4c16-460d-bc4a-75dc92669ddc with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 59aa0497-24a8-4770-be6a-c309f80fbc07- on 
agent 59aa0497-24a8-4770-be6a-c309f80fbc07-S0 at slave(462)@192.10.1.5:60456 
(windows-02.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net)
I0906 23:05:41.105769 13420 master.cpp:1251] Agent 
59aa0497-24a8-4770-be6a-c309f80fbc07-S0 at slave(462)@192.10.1.5:60456 
(windows-02.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net) disconnected
I0906 23:05:41.105769 13420 master.cpp:3267] Disconnecting agent 
59aa0497-24a8-4770-be6a-c309f80fbc07-S0 at slave(462)@192.10.1.5:60456 
(windows-02.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net)
I0906 23:05:41.105769 13420 master.cpp:3286] Deactivating agent 
59aa0497-24a8-4770-be6a-c309f80fbc07-S0 at slave(462)@192.10.1.5:60456 
(windows-02.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net)
I0906 23:05:41.106786 13000 hierarchical.cpp:359] Removed framework 
59aa0497-24a8-4770-be6a-c309f80fbc07-
I0906 23:05:41.106786 13000 hierarchical.cpp:795] Agent 
59aa0497-24a8-4770-be6a-c309f80fbc07-S0 deactivated
I0906 23:05:41.107771 13420 containerizer.cpp:2455] Destroying container 
9493fb9d-697f-416a-8979-2a164e62adfa in RUNNING state
I0906 23:05:41.107771 13420 containerizer.cpp:3118] Transitioning the state of 
container 9493fb9d-697f-416a-8979-2a164e62adfa from RUNNING to DESTROYING
I0906 23:05:41.107771 13420 launcher.cpp:166] Asked to destroy container 
9493fb9d-697f-416a-8979-2a164e62adfa
I0906 23:05:41.120775  9308 containerizer.cpp:2957] Container 
9493fb9d-697f-416a-8979-2a164e62adfa has exited
I0906 23:05:41.148789 13604 master.cpp:1093] Master terminating
I0906 23:05:[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (584 
ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (603 ms total)

[--] Global test environment tear-down
[==] 1051 tests from 103 test cases ran. (491325 ms total)
[  PASSED  ] 1050 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 1 FAILED TEST
  YOU HAVE 229 DISABLED TESTS

41.150792  9260 hierarchical.cpp:637] Removed agent 
59aa0497-24a8-4770-be6a-c309f80fbc07-S0
I0906 23:05:41.427780 13532 process.cpp:926] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On Sept. 6, 2018, 3:05 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68637/
> ---
> 
> (Updated Sept. 6, 2018, 3:05 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to remove ephemeral ports resources, we now perform an
> efficient swap to last + remove last operation.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3277b573b78d68a36d98bc8bee47f7a94a5aff18 
> 
> 
> Diff: https://reviews.apache.org/r/68637/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68637: Avoided extra copying of Resources in Master::offer.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On Sept. 6, 2018, 3:05 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68637/
> ---
> 
> (Updated Sept. 6, 2018, 3:05 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to remove ephemeral ports resources, we now perform an
> efficient swap to last + remove last operation.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3277b573b78d68a36d98bc8bee47f7a94a5aff18 
> 
> 
> Diff: https://reviews.apache.org/r/68637/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68660: Disabled SIGCHLD handling in the libev event loop.

[James Peach]

> On Sept. 6, 2018, 10:47 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/posix/libev/libev.cpp
> > Lines 84-89 (patched)
> > 
> >
> > * Let's use sigaction here rather than ::signal? See the portability 
> > notes here: http://man7.org/linux/man-pages/man2/signal.2.html
> > 
> > * Why do you need to set SIG_DFL? Can we just retrieve the old value 
> > and restore it later (note that sigaction makes this possible).
> > 
> > * Where's the race here? Can you clarify?
> > 
> > * Let's also remove the libev patch file if we have this proper 
> > approach to disabling it?
> > 
> > * Note that we have some stuff in os/signals.hpp, I don't know if you 
> > want to extend that stuff or not, either way seems fine to me.

> Let's use sigaction here rather than ::signal? See the portability notes 
> here: http://man7.org/linux/man-pages/man2/signal.2.html
> Why do you need to set SIG_DFL? Can we just retrieve the old value and 
> restore it later (note that sigaction makes this possible).

Setting `SIG_DFL` is not important, I'm just trying to sample the old handler 
value. You are right about using `sigaction` though, that's a better approach.

> Where's the race here? Can you clarify?

Just generically mentioning that manipulating the signal handling is racey.

> Let's also remove the libev patch file if we have this proper approach to 
> disabling it?

The patch causes the whole feature to be compiled out of libev, so it still 
results in a benefit (smaller code).

> Note that we have some stuff in os/signals.hpp, I don't know if you want to 
> extend that stuff or not, either way seems fine to me.

I'll probably use `sigaction` directly.


- James


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


On Sept. 6, 2018, 7:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> ---
> 
> (Updated Sept. 6, 2018, 7:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/posix/libev/libev.cpp 
> 173ee466e61d3a754202213b94645e125ce0421a 
> 
> 
> Diff: https://reviews.apache.org/r/68660/diff/1/
> 
> 
> Testing
> ---
> 
> configure with --with-libev=/usr, make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 68660: Disabled SIGCHLD handling in the libev event loop.

[Benjamin Mahler]

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




3rdparty/libprocess/src/posix/libev/libev.cpp
Lines 84-89 (patched)


* Let's use sigaction here rather than ::signal? See the portability notes 
here: http://man7.org/linux/man-pages/man2/signal.2.html

* Why do you need to set SIG_DFL? Can we just retrieve the old value and 
restore it later (note that sigaction makes this possible).

* Where's the race here? Can you clarify?

* Let's also remove the libev patch file if we have this proper approach to 
disabling it?

* Note that we have some stuff in os/signals.hpp, I don't know if you want 
to extend that stuff or not, either way seems fine to me.


- Benjamin Mahler


On Sept. 6, 2018, 7:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> ---
> 
> (Updated Sept. 6, 2018, 7:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/posix/libev/libev.cpp 
> 173ee466e61d3a754202213b94645e125ce0421a 
> 
> 
> Diff: https://reviews.apache.org/r/68660/diff/1/
> 
> 
> Testing
> ---
> 
> configure with --with-libev=/usr, make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 68662: Avoided double copying of outgoing messages in the master.

[Benjamin Mahler]

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

Review request for mesos, Gastón Kleiman, Gilbert Song, and Greg Mann.


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


Repository: mesos


Description
---

When framework metrics were introduced, there was an evolve + devolve
conversion occuring in order to map old style messages to the new
`scheduler::Event::Type`. This results in copying all outgoing
messages twice.

This patch avoids the copying by adding overloads for the old style
messages.


Diffs
-

  src/master/master.hpp ec2ae01ec813ff7fcb0e2a3d0dbad3c3f9993a98 
  src/master/metrics.hpp 14f894a40e96be93037e0d262ddc4b04a6fcc301 
  src/master/metrics.cpp 655c75a042a3bfeb264a55d7495f957c407a4d6d 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 68637: Avoided extra copying of Resources in Master::offer.

[Benjamin Mahler]

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

(Updated Sept. 6, 2018, 10:05 p.m.)


Review request for mesos, Gastón Kleiman and Meng Zhu.


Changes
---

Updated to use swap to last + remove last.


Summary (updated)
-

Avoided extra copying of Resources in Master::offer.


Repository: mesos


Description (updated)
---

In order to remove ephemeral ports resources, we now perform an
efficient swap to last + remove last operation.


Diffs (updated)
-

  src/master/master.cpp 3277b573b78d68a36d98bc8bee47f7a94a5aff18 


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

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

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

(Updated Sept. 6, 2018, 2:56 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón Kleiman.


Changes
---

Addressed Benjamin's comment.


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


Repository: mesos


Description
---

This patch lets `class Resources` only store shared
pointers to the underlying resource objects, so that
read-only filter operations such as `reserved()`,
`unreserved()` and etc. can avoid making copies of
the whole resource objects. Instead, only shared pointers
are copied.

In write operations, we check if there are more than one
references to the resource object. If so, a copy is made
for safe mutation without affecting owners.

To maintain the usage abstraction that `class Resources`
still holds resource objects, we utilize
`boost::indirect_iterator` iterator adapter to deference
the shared pointers as we iterate.


Diffs (updated)
-

  include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
  include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
  src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
  src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 


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

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


Testing
---

make check

Benchmarked on a Ubuntu box using clang -O2 optimized build with an i5 
processor, frequency fixed at 1.6GHz.

Resources benchmarks (Measure geo-mean of various operations):

Normalized Geomean latency

Master  Copy-on-write
Arithmetic  1   1.09
Filter  1   0.28
Contains1   0.27
Parse   1   1.01

Sample allocator benchmark (Measure first allocation latency):

HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1 (1k agents, 50 frameworks):

Master 1
Copy-on-write 0.72

HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1 (1k agents, 50 
frameworks):

Master 1
Copy-on-write 0.72


Thanks,

Meng Zhu

Re: Review Request 68637: Avoided common-case extra copying of Resources in Master::offer.

[Benjamin Mahler]

> On Sept. 6, 2018, 4:42 a.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Line 9379 (original), 9378 (patched)
> > 
> >
> > `DeleteSubrange` will affect all the elements after `i`, this may cause 
> > quite some overhead.
> > 
> > We can swap `ephemeral_ports` resources to the tail and 
> > `DeleteSubrange` them in one go.
> > 
> > I do not know how often ephemeral ports are used, but the above 
> > approach seems to be worthwhile and would not hurt readability much.

Yeah I originally was planning to do a Swap+RemoveLast approach, but didn't 
think I could write the code cleanly. It does seem like trivial tweak to the 
current patch, I'll send out an update.


- Benjamin


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


On Sept. 5, 2018, 10:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68637/
> ---
> 
> (Updated Sept. 5, 2018, 10:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's more typical for there not to be ephemeral ports resources,
> therefore we avoid copying in the common case.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 
> 
> 
> Diff: https://reviews.apache.org/r/68637/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68660: Disabled SIGCHLD handling in the libev event loop.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68660 was successfully built and tested.

Reviews applied: `['68660']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2304/mesos-review-68660

- Mesos Reviewbot Windows


On Sept. 6, 2018, 7:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> ---
> 
> (Updated Sept. 6, 2018, 7:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/posix/libev/libev.cpp 
> 173ee466e61d3a754202213b94645e125ce0421a 
> 
> 
> Diff: https://reviews.apache.org/r/68660/diff/1/
> 
> 
> Testing
> ---
> 
> configure with --with-libev=/usr, make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 68660: Disabled SIGCHLD handling in the libev event loop.

[James Peach]

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

Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.


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


Repository: mesos


Description
---

libev will consume SIGCHLD signals by default, which interferes
with a number of libprocess assumptions around dealing with child
processes. The recommended way to disable this behavior is to reset
the SIGCHLD signal handler after initializing the libeve default
event loop.


Diffs
-

  3rdparty/libprocess/src/posix/libev/libev.cpp 
173ee466e61d3a754202213b94645e125ce0421a 


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


Testing
---

configure with --with-libev=/usr, make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 68644: Closed all file descriptors except `whitelist_fds` in posix/subprocess.

[James Peach]

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




3rdparty/libprocess/src/posix/subprocess.hpp
Lines 195 (patched)


We need to be careful here. We are in an async-signal-safe context but 
hashmap and list allocate memory.

Actually iterating over the directory is difficult to do in an 
async-signal-safe context. You can open the directory in the parent and do the 
iteration in the child, but I don't think there's any guarantee that the 
readdir in the child is safe (though AFAIK in glibc it would work).


- James Peach


On Sept. 6, 2018, 1:25 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68644/
> ---
> 
> (Updated Sept. 6, 2018, 1:25 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Closed all file descriptors except `whitelist_fds` in posix/subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/posix/subprocess.hpp 
> 007058b61fdcd4716aa793516c842c3cef8c0a29 
>   3rdparty/libprocess/src/subprocess.cpp 
> c0640de2dc4278b884282dfaad98c49c3b067a5b 
> 
> 
> Diff: https://reviews.apache.org/r/68644/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 68643: Updated `MesosContainerizerLaunch` to call `os::lsof()`.

[James Peach]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/launch.cpp
Lines 1141 (patched)


This code was previously before the fork so that is didn't do memory 
allocation in an async-signal-safe context (i.e. after fork but before exec).


- James Peach


On Sept. 6, 2018, 1:25 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68643/
> ---
> 
> (Updated Sept. 6, 2018, 1:25 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `MesosContainerizerLaunch` to call `os::lsof()`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 7193da0a094df3e441e185c62b3a0379a0bdc4a2 
> 
> 
> Diff: https://reviews.apache.org/r/68643/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 68642: Added `lsof()` into stout.

[James Peach]

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



We should add a basic test for this, even if it just ensures that the result 
contains the elements 0, 1 and 2.


3rdparty/stout/include/stout/os/lsof.hpp
Lines 16 (patched)


Is this really a double newline? I can't keep track, but this seems 
inconsistent with the other files in this patch.

I looked in the current headers and found 3 variants of how many blank 
lines files like this should have :)



3rdparty/stout/include/stout/os/posix/lsof.hpp
Lines 29 (patched)


Why hashset rather than vector? By definition there aren't any duplicates, 
so we don't need the overhead of hashset here?

/cc @bmahler



3rdparty/stout/include/stout/os/posix/lsof.hpp
Lines 35 (patched)


We don't actually need any #ifdefs here. On Linux, /dev/fd is a symlink to 
/proc/self/fd, so we can just unconditionally list /dev/fd and handle the error 
for platforms where this is missing.



3rdparty/stout/include/stout/os/posix/lsof.hpp
Lines 46 (patched)


Add the fd string into the error message?



3rdparty/stout/include/stout/os/windows/lsof.hpp
Lines 23 (patched)


In other case, we simply omitted the Windows version altogether, e.g. 
https://reviews.apache.org/r/68398/

/cc @andschwa


- James Peach


On Sept. 6, 2018, 1:25 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68642/
> ---
> 
> (Updated Sept. 6, 2018, 1:25 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `lsof()` into stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am a9c61fcba253a811494cdcdb0afb3d3a018f4585 
>   3rdparty/stout/include/stout/os.hpp 
> aee041891b7e7ff93a0b1ac31019a7a3d4eae962 
>   3rdparty/stout/include/stout/os/lsof.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/lsof.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/lsof.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68642/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65168: Fixed HTTP errors caused by dropped HTTP responses by IOSwitchboard.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65168 was successfully built and tested.

Reviews applied: `['68232', '68230', '68231', '65168']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2303/mesos-review-65168

- Mesos Reviewbot Windows


On Sept. 6, 2018, 3:22 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65168/
> ---
> 
> (Updated Sept. 6, 2018, 3:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Qian Zhang.
> 
> 
> Bugs: MESOS-8545
> https://issues.apache.org/jira/browse/MESOS-8545
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, IOSwitchboard process could terminate before all HTTP
> responses had been sent to the agent. In the case of
> `ATTACH_CONTAINER_INPUT` call, we could drop a final HTTP `200 OK`
> response, so the agent got broken HTTP connection for the call.
> This patch introduces an acknowledgment for the received response
> for the `ATTACH_CONTAINER_INPUT` call. This acknowledgment is a new
> type of control messages for the `ATTACH_CONTAINER_INPUT` call. When
> IOSwitchboard receives an acknowledgment, and io redirects are
> finished, it terminates itself. That guarantees that the agent always
> receives a response for the `ATTACH_CONTAINER_INPUT` call.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 52b0e521ed1c651c90b3a3df7c4df576288bf400 
>   src/slave/http.hpp 78200874cb03a3d5af4ee0443c03a1d94cd494cc 
>   src/slave/http.cpp fb923686e92ff635e99cc2ccc6316f0282ab3e3a 
> 
> 
> Diff: https://reviews.apache.org/r/65168/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65168: Fixed HTTP errors caused by dropped HTTP responses by IOSwitchboard.

[Andrei Budnik]

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

(Updated Sept. 6, 2018, 3:22 p.m.)


Review request for mesos, Alexander Rukletsov and Qian Zhang.


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


Repository: mesos


Description
---

Previously, IOSwitchboard process could terminate before all HTTP
responses had been sent to the agent. In the case of
`ATTACH_CONTAINER_INPUT` call, we could drop a final HTTP `200 OK`
response, so the agent got broken HTTP connection for the call.
This patch introduces an acknowledgment for the received response
for the `ATTACH_CONTAINER_INPUT` call. This acknowledgment is a new
type of control messages for the `ATTACH_CONTAINER_INPUT` call. When
IOSwitchboard receives an acknowledgment, and io redirects are
finished, it terminates itself. That guarantees that the agent always
receives a response for the `ATTACH_CONTAINER_INPUT` call.


Diffs (updated)
-

  include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
  src/slave/containerizer/mesos/io/switchboard.cpp 
52b0e521ed1c651c90b3a3df7c4df576288bf400 
  src/slave/http.hpp 78200874cb03a3d5af4ee0443c03a1d94cd494cc 
  src/slave/http.cpp fb923686e92ff635e99cc2ccc6316f0282ab3e3a 


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

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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68568: Added '/roles' to the set of batched master endpoints.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/master/http.cpp
Line 2605 (original), 2562 (patched)


I'll move it to `master.cpp`.



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


I think this can be removed now.


- Alexander Rukletsov


On Sept. 5, 2018, 12:18 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68568/
> ---
> 
> (Updated Sept. 5, 2018, 12:18 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-9194
> https://issues.apache.org/jira/browse/MESOS-9194
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For improved consistency, the '/roles' endpoint on the
> master is now marked as read-only and uses the batching
> mechanism shared by the other read-only endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 
>   src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b 
>   src/master/readonly_handler.cpp 47d7de5bce4f6b21134596fe53dd02e457f9c069 
> 
> 
> Diff: https://reviews.apache.org/r/68568/diff/2/
> 
> 
> Testing
> ---
> 
> Successful internal CI run executing `make check` on several platforms. 
> (Jenkins id #4290)
> 
> For rev. 2, internal CI run #4340.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 68236: Fixed `LaunchNestedContainerSessionsInParallel` test.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Sept. 3, 2018, 4:58 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68236/
> ---
> 
> (Updated Sept. 3, 2018, 4:58 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-9116
> https://issues.apache.org/jira/browse/MESOS-9116
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we sent `ATTACH_CONTAINER_OUTPUT` to attach to a
> short-living nested container. An attempt to attach to a terminated
> nested container leads to HTTP 500 error. This patch gets rid of
> `ATTACH_CONTAINER_OUTPUT` in favor of `LAUNCH_NESTED_CONTAINER_SESSION`
> so that we can read the container's output without using an extra call.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 43541af732df271177f0fd56ddc419adec461110 
> 
> 
> Diff: https://reviews.apache.org/r/68236/diff/1/
> 
> 
> Testing
> ---
> 
> 1. sudo make check (Fedora 25)
> 2. internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 68639: Renamed `Resources::resources` to `noMutationWithoutExclusiveOwnership`.

[Benjamin Bannier]

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




include/mesos/v1/resources.hpp
Lines 695-696 (original), 704-705 (patched)


This sounds like a commit message and less like documentation for the 
current state.

Maybe just something like this?

We name the `vector` field `noMutationWithoutExclusiveOwnership` ...



include/mesos/v1/resources.hpp
Line 703 (original), 714 (patched)


I like how transparent this naming makes the need for future improvements, 
but it would be great if it would still transport that this is a container of 
`Resource_`.

Something like 
`resources[NoMutationWithoutExclusiveOwnership|Unsafe|NotConcurrencySafe|...]` 
would make the resulting usages more readable.

We could also accompany this with a name for the type users could use, e.g.,

using Resources_Unsafe = std::shared_ptr;

These could be used now in places we'd use cow wrappers once the `TODO` 
gets implemented.


- Benjamin Bannier


On Sept. 6, 2018, 2:13 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68639/
> ---
> 
> (Updated Sept. 6, 2018, 2:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón 
> Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to the copy-on-write optimization (MESOS-6765), one needs to
> check the `use_count` of `Resource_` before mutating. Currently,
> there is no mechanism to enforce this. As a short-term mitigation
> measure, we rename `resources` to `noMutationWithoutExclusiveOwnership`
> to alert people about obtaining an exclusive ownership before mutating
> the `Resource_` objects.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
>   include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
>   src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
>   src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 
> 
> 
> Diff: https://reviews.apache.org/r/68639/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Benjamin Bannier]

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




include/mesos/resources.hpp
Line 588 (original), 590 (patched)


`s/Resource/Resource_/`?



include/mesos/resources.hpp
Lines 596-597 (patched)


I'd leave this above above the iterator types as documentation.



include/mesos/resources.hpp
Lines 691-694 (patched)


nit: While this is likely true, copying a shared pointer can be suprisingly 
non-inexpensive due to copying of its atomic ref count (likely protected with 
memory barries, and copies triggering cache invalidations across threads 
holding ref counts).

Your benchmarks strongly suggest that we still benefit from this patch.



src/v1/resources.cpp
Lines 2117-2118 (original), 2154-2178 (patched)


This function looks redundant and identical to `Resources::add(const 
Resource&)` to me. It also introduces another place where we leak 
implementation details into the interface.

Suggest to remove and instead let callers perform the deref. We could 
always add something similar back once we want to support `move`'ing a `that`.


- Benjamin Bannier


On Sept. 5, 2018, 11:47 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68490/
> ---
> 
> (Updated Sept. 5, 2018, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón 
> Kleiman.
> 
> 
> Bugs: MESOS-6765
> https://issues.apache.org/jira/browse/MESOS-6765
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch lets `class Resources` only store shared
> pointers to the underlying resource objects, so that
> read-only filter operations such as `reserved()`,
> `unreserved()` and etc. can avoid making copies of
> the whole resource objects. Instead, only shared pointers
> are copied.
> 
> In write operations, we check if there are more than one
> references to the resource object. If so, a copy is made
> for safe mutation without affecting owners.
> 
> To maintain the usage abstraction that `class Resources`
> still holds resource objects, we utilize
> `boost::indirect_iterator` iterator adapter to deference
> the shared pointers as we iterate.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
>   include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
>   src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
>   src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 
> 
> 
> Diff: https://reviews.apache.org/r/68490/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarked on a Ubuntu box using clang -O2 optimized build with an i5 
> processor, frequency fixed at 1.6GHz.
> 
> Resources benchmarks (Measure geo-mean of various operations):
> 
> Normalized Geomean latency
>   
>   Master  Copy-on-write
> Arithmetic1   1.09
> Filter1   0.28
> Contains  1   0.27
> Parse 1   1.01
> 
> Sample allocator benchmark (Measure first allocation latency):
> 
> HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1 (1k agents, 50 
> frameworks):
> 
> Master 1
> Copy-on-write 0.72
> 
> HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1 (1k agents, 50 
> frameworks):
> 
> Master 1
> Copy-on-write 0.72
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68640: Added version check and bundling of libevent.

[Till Toenshoff via Review Board]

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

(Updated Sept. 6, 2018, 10:07 a.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.


Changes
---

Fixed missing binaries.


Summary (updated)
-

Added version check and bundling of libevent.


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


Repository: mesos


Description (updated)
---

Bundles libevent 2.0.22 to ensure functional SSL builds accross all
supported platforms. Namely macOS and Ubuntu have shown problems when
using more recent libevent versions. The underlying problem has been
under investigation for a longer period of time - so far without a
solution. The bundled libevent includes a patch to make it libssl
version > 1.0.x compatible. That patch has been extracted from the
Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
builds against both, libssl 1.0.x as well as libssl 1.1.x.
For unbundled builds a version detection of known problematic
distributions vs. provided libevent is included.


Diffs (updated)
-

  3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
  3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
  3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
  3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
  configure.ac 9de51e59796e2a64138acb82cbbb080f1ae6cc6e 
  src/python/native_common/ext_modules.py.in 
7efbb8ec91b74c32ac629f1f2e7de82986d518db 


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

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


Testing
---

Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing is 
ongoing. CMake updates will follow.


Thanks,

Till Toenshoff

Re: Review Request 68654: Enabled isort for src/python/lib.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68654 was successfully built and tested.

Reviews applied: `['68654']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2302/mesos-review-68654

- Mesos Reviewbot Windows


On Sept. 6, 2018, 7:37 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68654/
> ---
> 
> (Updated Sept. 6, 2018, 7:37 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled isort for src/python/lib.
> 
> 
> Diffs
> -
> 
>   src/python/lib/mesos/http.py cd1587797db7d75c6b839851f0f3e5671269307c 
>   src/python/lib/setup.cfg PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py 
> 096eab82311e02b0fa829101b6ad1b23e42ce088 
>   src/python/lib/tests/test_http.py 41a52f511318cb69c8e9976f09e2f142327544ab 
>   src/python/lib/tox.ini 3ee77681a9b802cd5b4a7910779b8d50aac4cf69 
> 
> 
> Diff: https://reviews.apache.org/r/68654/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 68641: Added version check and bundling of libevent within libprocess.

[Benjamin Bannier]

> On Sept. 6, 2018, 3:23 a.m., Mesos Reviewbot Windows wrote:
> > FAIL: Failed to apply the dependent review: 68640.
> > 
> > Failed command: `python.exe .\support\apply-reviews.py -n -r 68640`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2299/mesos-review-68641
> > 
> > Relevant logs:
> > 
> > - 
> > [apply-review-68640.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2299/mesos-review-68641/logs/apply-review-68640.log):
> > 
> > ```
> > ?68640.patch:77: trailing whitespace.
> >  
> > 68640.patch:90: space before tab in indent.
> > return 1;
> > 68640.patch:92: trailing whitespace.
> >  
> > 68640.patch:95: space before tab in indent.
> > if (!b)
> > 68640.patch:96: space before tab in indent.
> > return 0;
> > error: missing binary patch data for 
> > '3rdparty/libevent-2.0.22-stable.tar.gz'
> > error: binary patch does not apply to 
> > '3rdparty/libevent-2.0.22-stable.tar.gz'
> > error: 3rdparty/libevent-2.0.22-stable.tar.gz: patch does not apply
> > ```

@till: Could you upload the binaries as well? See e.g., 
https://issues.apache.org/jira/browse/MESOS-3906?focusedCommentId=15003432&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-15003432
 how to accomplish this.


- Benjamin


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


On Sept. 6, 2018, 2:32 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68641/
> ---
> 
> (Updated Sept. 6, 2018, 2:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.
> 
> 
> Bugs: MESOS-7076
> https://issues.apache.org/jira/browse/MESOS-7076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added version check and bundling of libevent within libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am f9d9d0609f5c8fed419be83319319e67afc063b7 
>   3rdparty/libprocess/configure.ac e9e2434b7b6fe6c94c28744a86f9412b6302cbe0 
> 
> 
> Diff: https://reviews.apache.org/r/68641/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing 
> is ongoing. CMake updates will follow.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 68654: Enabled isort for src/python/lib.

[Eric Chung]

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

Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description
---

Enabled isort for src/python/lib.


Diffs
-

  src/python/lib/mesos/http.py cd1587797db7d75c6b839851f0f3e5671269307c 
  src/python/lib/setup.cfg PRE-CREATION 
  src/python/lib/tests/test_exceptions.py 
096eab82311e02b0fa829101b6ad1b23e42ce088 
  src/python/lib/tests/test_http.py 41a52f511318cb69c8e9976f09e2f142327544ab 
  src/python/lib/tox.ini 3ee77681a9b802cd5b4a7910779b8d50aac4cf69 


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


Testing
---


Thanks,

Eric Chung

Review Request 68653: Enabled isort for src/python/lib.

[Eric Chung]

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

Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description
---

Enabled isort for src/python/lib.


Diffs
-

  src/python/lib/mesos/http.py 073c159dc6916fa5d7349a033db022d7175aef05 
  src/python/lib/setup.cfg PRE-CREATION 
  src/python/lib/tests/test_exceptions.py 
096eab82311e02b0fa829101b6ad1b23e42ce088 
  src/python/lib/tests/test_http.py 66dd6d7c6272d1828dd591829ca3543d3430f69b 
  src/python/lib/tox.ini fd5e89c77c8608fea21e9caad814c6e111ad57db 


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


Testing
---


Thanks,

Eric Chung