Re: Review Request 66996: Removed the OpenSSL dependency for building gRPC in Mesos.

[Chun-Hung Hsiao]

> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > 3rdparty/Makefile.am
> > Lines 326 (patched)
> > 
> >
> > This variable name makes sense only 50% of the time, how about e.g., 
> > `GRPC_LIB_SUFFIX`?

I'll use `GRPC_VARIANT`.


> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > 3rdparty/Makefile.am
> > Lines 341-350 (original), 339-348 (patched)
> > 
> >
> > It is very hard to see what is actually being execute here in what 
> > environment. Could you reflow this to e.g., always pass the env after 
> > `make`, reorder the variables and reflow the code?
> > 
> > $(MAKE) $(AM_MAKEFLAGS) \
> >   $(LIB_GRPC:%=$(abs_builddir)/%)   \
> >   CC="$(CC)"\
> >   CXX="$(CXX)"  \
> >   LD="$(CC)"\
> >   LDXX="$(CXX)" \
> >   CPPFLAGS="$(PROTOBUF_INCLUDE_FLAGS)   \
> > $(SSL_INCLUDE_FLAGS)\
> > $(ZLIB_INCLUDE_FLAGS)"  \
> >   LDFLAGS="$(PROTOBUF_LINKER_FLAGS) \
> >$(SSL_LINKER_FLAGS)  \
> >$(ZLIB_LINKER_FLAGS)"\
> >   HAS_PKG_CONFIG=false  \
> >   NO_PROTOC=false   \
> >   PROTOC="$(PROTOC)"

gRPC's makefile uses target-specific variable assignments 
(https://www.gnu.org/software/make/manual/html_node/Target_002dspecific.html) 
for appending `CPPFLAGS`. It seems that values taken from the command line will 
take precedence and the append will be ignored, and this is why I assign 
`CPPFLAGS` in the environment. Will add a comment about this.


> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > 3rdparty/Makefile.am
> > Lines 341-342 (original), 339-340 (patched)
> > 
> >
> > Why are we removing these extra flags? They don't seem to come from the 
> > normal flags?

Those flags are no longer required in grpc v1.10. Will use another patch to 
remove them first.


> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 175 (patched)
> > 
> >
> > Why are we removing these extra flags? They don't seem to come from the 
> > normal flags?

The SSL flags should have already been set up in `LIBS` by `configure.ac` when 
SSL is enabled:
https://github.com/apache/mesos/blob/master/configure.ac#L1864


> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Line 182 (original), 185 (patched)
> > 
> >
> > It is not clear to me that this would always find the correct 
> > libraries. I see e.g., that the cmake build does not seem to use the 
> > `_unsecure` suffix even when the libraries where built without SSL.
> > 
> > Do we need to explicitly discover these libraries during configure time 
> > to make sure we don't risk failing to link later?

We already did: https://github.com/apache/mesos/blob/master/configure.ac#L2072. 
The assumption here is that for custom grpc, we assume the whole grpc package 
is installed so the standard grpc library must be there.


> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > src/python/native_common/ext_modules.py.in
> > Line 132 (original), 132 (patched)
> > 
> >
> > This variable name makes sense only 50% of the time, how about e.g., 
> > `grpc_lib_suffix`?

I'll use `grpc_variant`.


> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > src/python/native_common/ext_modules.py.in
> > Lines 151-152 (original)
> > 
> >
> > How do we manage to link an object file (which has no dependency 
> > information) when SSL is enabled?

These SSL flags should have already been set up in `LIBS` by `configure.ac` 
when SSL is enabled:
https://github.com/apache/mesos/blob/master/configure.ac#L1864
https://github.com/apache/mesos/blob/master/src/Makefile.am#L2196
https://github.com/apache/mesos/blob/master/src/python/native_common/ext_modules.py.in#L168


- Chun-Hung


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

Review Request 67012: Resolve container target paths within their rootfs.

[Jason Lai]

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

Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, James Peach, and 
Zhitao Li.


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


Repository: mesos


Description
---

Resolve container target paths within their rootfs.


Diffs
-

  src/slave/containerizer/mesos/launch/linux_helper.cpp PRE-CREATION 


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


Testing
---


Thanks,

Jason Lai

Re: Review Request 66982: Added a `decline` flag in `recoverResources` in the allocator.

[Benjamin Mahler]

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




include/mesos/allocator/allocator.hpp
Lines 372 (patched)


Can you make this a Reason enum? E.g.

ACCEPT_REMAINDER,
OFFER_TIMEOUT,
DECLINE,
WORKLOAD_TERMINATED,
etc

Then can the allocator make the call about which cases should be considered 
for the exclusion set change?


- Benjamin Mahler


On May 7, 2018, 6:09 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66982/
> ---
> 
> (Updated May 7, 2018, 6:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `decline` flag acts as an hint to the allocator that this resource
> recover is triggered by the framework "declining" the offered resources.
> Currently this includes: framework `DECLINE` calls, framework `ACCEPT`
> calls with filters explicitly set and offer timeouts. Note, framework
> `ACCEPT` calls with unset filters (i.e. default filters) are not
> considered as "declining" offered resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> e5a5355646c834280008867e89eb258eaefc2f1d 
>   src/master/allocator/mesos/allocator.hpp 
> c453c015b234deff7efd00269da25dcec8cbf1ae 
>   src/master/allocator/mesos/hierarchical.hpp 
> 955ae3e6a9e3c790fb260311ec4b4ef725a826d3 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1000968be6a2935a4cac571414d7f06d7df7acf0 
>   src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 
>   src/tests/allocator.hpp 341efa665ad0ce897e087fb8d73ec50fd041d559 
>   src/tests/master_allocator_tests.cpp 
> e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
>   src/tests/slave_recovery_tests.cpp afe8b8a6cc21421a37164016d7fe01bf96a559da 
> 
> 
> Diff: https://reviews.apache.org/r/66982/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67009: Added tests of resource provider registrar recovery.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67009]

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

- Mesos Reviewbot


On May 8, 2018, 3:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67009/
> ---
> 
> (Updated May 8, 2018, 3:16 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8837
> https://issues.apache.org/jira/browse/MESOS-8837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests of resource provider registrar recovery.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_provider_manager_tests.cpp 
> c2045f2ba24f3e4b959115f23b706e733a75fea8 
> 
> 
> Diff: https://reviews.apache.org/r/67009/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 67018: Removed unnecessary gRPC build flags in libprocess.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

The `-Wno-deprecated-declarations` and `-Wno-unused-function` flags are
no longer required to build the bundled gRPC 1.10.0.


Diffs
-

  3rdparty/libprocess/3rdparty/Makefile.am 
7fb77f8f29e00a276cfb0efc5324ac838c8b5176 


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


Testing
---

Tests done later in chain.


Thanks,

Chun-Hung Hsiao

Review Request 67017: Removed unnecessary gRPC build flags in Mesos.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

The `-Wno-deprecated-declarations` and `-Wno-unused-function` flags are
no longer required to build the bundled gRPC 1.10.0.


Diffs
-

  3rdparty/Makefile.am 8d9fa85dd65a94d91670d54dab36deea80d14996 


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


Testing
---

Tests done later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65900: Defer creation of volume target paths to container launch.

[Jason Lai]

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

(Updated May 8, 2018, 7:07 p.m.)


Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James 
Peach, and Zhitao Li.


Changes
---

Refactored and extracted bidirectional mount propagation code to `utils.hpp`.


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


Repository: mesos


Description
---

Defer creation of volume target paths to container launch.


Diffs (updated)
-

  src/Makefile.am a2ba1e6de0868c7a4fe58304ec479dce039bc288 
  src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a 
  src/slave/containerizer/mesos/isolators/volume/image.cpp 
8ca3d55d5792ed1cfc8e49df40587ac2abc83fec 
  src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 


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

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


Testing
---


Thanks,

Jason Lai

Re: Review Request 66993: Removed unnecessary expectation on termination.

[Zhitao Li]

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

(Updated May 8, 2018, 1:36 p.m.)


Review request for mesos, Andrei Budnik and James Peach.


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


Repository: mesos


Description
---

This test was flaky because termination could already happened when we
set up the expectation. Given that we already verified task state, I do
not see checking container termination explicitly is necessary, so
removing the expectation should fix the flakiness.


Diffs (updated)
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
d834e531a550028cd57ac409c9312dd22138e8d5 


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

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


Testing
---

GTEST_FILTER="DockerContainerizerTest.ROOT_DOCKER_MaxCompletionTime" 
./bin/mesos-tests.sh --gtest_repeat=100 --verbose


Thanks,

Zhitao Li

Re: Review Request 66993: Removed unnecessary expectation on termination.

[Andrei Budnik]

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


Ship it!




Ship It!

- Andrei Budnik


On May 8, 2018, 8:36 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66993/
> ---
> 
> (Updated May 8, 2018, 8:36 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and James Peach.
> 
> 
> Bugs: MESOS-8884
> https://issues.apache.org/jira/browse/MESOS-8884
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test was flaky because termination could already happened when we
> set up the expectation. Given that we already verified task state, I do
> not see checking container termination explicitly is necessary, so
> removing the expectation should fix the flakiness.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> d834e531a550028cd57ac409c9312dd22138e8d5 
> 
> 
> Diff: https://reviews.apache.org/r/66993/diff/2/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="DockerContainerizerTest.ROOT_DOCKER_MaxCompletionTime" 
> ./bin/mesos-tests.sh --gtest_repeat=100 --verbose
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66822: Added per Framework Calls to metrics.

[Gaston Kleiman]

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




src/master/metrics.cpp
Lines 556-557 (patched)


Fits in one line.



src/master/metrics.cpp
Lines 595-623 (patched)


We could replace these two methods with a single method that that takes a 
`const scheduler::Call::Type&` instead of a `const scheduler::Call&`.

We use this pattern for a lot of metrics, so should consider adding 
something like:

```
template 
class EnumCounter
{
  EnumCounter(const lambda::function 
_keyNameGenerator);
  
  ~EnumCounter();
  
  void increment(const Type& type);
  
private:
  hashmap counters;
  const lambda::function keyNameGenerator;
}
```


- Gaston Kleiman


On April 30, 2018, 11:45 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66822/
> ---
> 
> (Updated April 30, 2018, 11:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per Framework Calls to metrics.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad 
>   src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
> 
> 
> Diff: https://reviews.apache.org/r/66822/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66861: Added per-framework DRF position metrics to the allocator.

[Greg Mann]

> On May 1, 2018, 12:39 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1766 (patched)
> > 
> >
> > Do we need the typedef up here? or should we put it down to the foreach 
> > loop to be clear it's needed there? (if that's why it was added)

Good call, makes more sense to move it down.


> On May 1, 2018, 12:39 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1775-1790 (patched)
> > 
> >
> > Maybe something like this?
> > 
> > ```
> > auto frameworks = [role];
> > auto framework = frameworks.find(frameworkId);
> > 
> > if (framework == frameworks.end()) {
> >   framework = frameworks.emplace(frameworkId, {position, 
> > position}).second;
> > }
> > 
> > framework.first = std::min(framework.first, position);
> > framework.second = std::max(framework.second, position);
> > ```
> > 
> > Or:
> > 
> > ```
> > auto frameworks = [role];
> > auto framework = frameworks.find(frameworkId);
> > 
> > if (framework == frameworks.end()) {
> >   frameworks.emplace(frameworkId, {position, position});
> > } else {
> >   framework.first = std::min(framework.first, position);
> >   framework.second = std::max(framework.second, position);
> > }
> > ```
> > 
> > This also avoids most of the excessive map lookups while also making it 
> > more readable?

Definite improvement; thanks!


> On May 1, 2018, 12:39 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1968-1970 (original), 2005-2007 (patched)
> > 
> >
> > We break the framework id loop here, which I think screws up the min / 
> > max calculation? Ideally we would also break the role loop if nothing is 
> > allocatable, but we don't do that today (hopefully we could make it easy to 
> > add the latter breaking without the metrics getting screwed up?)
> > 
> > Ditto below.

Thanks for catching this. I elected to store the sorted frameworks in a local 
variable and increment metrics immediately, so we should be fine in the future 
regardless of what kind of breaking we decide to do.


- Greg


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


On May 8, 2018, 10:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66861/
> ---
> 
> (Updated May 8, 2018, 10:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During each allocation cycle, the allocator re-sorts roles and
> frameworks for each agent in the cluster. This means that for each
> agent there exists a total order of (role, framework) tuples.
> 
> This patch adds per-framework, per-role metrics which track the
> minimum and maximum positions attained by the framework in this
> sorting process, from the most recent allocation cycle.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1000968be6a2935a4cac571414d7f06d7df7acf0 
>   src/master/allocator/mesos/metrics.hpp 
> 6d386225c301d5ab44f3cc0ecdd1478fb5162e5b 
>   src/master/allocator/mesos/metrics.cpp 
> 5194f5b3b3f507b36f02300775230157db3dd477 
> 
> 
> Diff: https://reviews.apache.org/r/66861/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 66861: Added per-framework DRF position metrics to the allocator.

[Greg Mann]

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

(Updated May 8, 2018, 10:18 p.m.)


Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
Vinod Kone.


Repository: mesos


Description
---

During each allocation cycle, the allocator re-sorts roles and
frameworks for each agent in the cluster. This means that for each
agent there exists a total order of (role, framework) tuples.

This patch adds per-framework, per-role metrics which track the
minimum and maximum positions attained by the framework in this
sorting process, from the most recent allocation cycle.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
1000968be6a2935a4cac571414d7f06d7df7acf0 
  src/master/allocator/mesos/metrics.hpp 
6d386225c301d5ab44f3cc0ecdd1478fb5162e5b 
  src/master/allocator/mesos/metrics.cpp 
5194f5b3b3f507b36f02300775230157db3dd477 


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

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


Testing
---


Thanks,

Greg Mann

Re: Review Request 66841: Added a hash function for 'Duration'.

[Greg Mann]

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

(Updated May 8, 2018, 11:18 p.m.)


Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
Vinod Kone.


Repository: mesos


Description
---

Added a hash function for 'Duration'.


Diffs (updated)
-

  3rdparty/stout/include/stout/duration.hpp 
42c43cda21c75fc3bef962af67c4a09df68a95af 


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

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


Testing
---


Thanks,

Greg Mann

Review Request 67013: Sort container mounts by their target paths.

[Jason Lai]

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

Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, James Peach, and 
Zhitao Li.


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


Repository: mesos


Description
---

Sort container mounts by their target paths.


Diffs
-

  src/slave/containerizer/mesos/launch/linux_helper.cpp PRE-CREATION 


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


Testing
---


Thanks,

Jason Lai

Re: Review Request 66984: Added the ability to exhaustively allocate declined resources.

[Benjamin Mahler]

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



The description could use some more explanation around allocating up to quota 
guarantee vs above guarantee up to limit, and how these pertain to 
exhaustiveness. This also seems to have an implication on the code (e.g. if 
quota check for candidates). I already forgot what the thinking is there so I 
certainly won't remember a week from now :)


src/master/allocator/mesos/hierarchical.hpp
Lines 440-445 (patched)


I think we can just update the above paragraph to clarify it's per role, 
rather than having it as a separate note here.



src/master/allocator/mesos/hierarchical.hpp
Lines 447-448 (patched)


Let's clearly specify the conditions under which it gets cleared, is this 
the only event outside of the set filling up?

I'm inclined to clear filters for the agent attribute change case but keep 
it in the exclusion set, so that we don't hit the starvation problem this 
feature aims to address.



src/master/allocator/mesos/hierarchical.hpp
Lines 450-451 (patched)


Let's note here that if introduce cases such that we clear the set too 
frequently we run into the problem this feature aims to address.



src/master/allocator/mesos/hierarchical.hpp
Lines 452 (patched)


hashset? If not, can you clarify where we rely on the ordering?



src/master/allocator/mesos/hierarchical.cpp
Lines 652-654 (patched)


See my comment above.



src/master/allocator/mesos/hierarchical.cpp
Lines 1215-1217 (patched)


It's a little odd that it's not cleared here but it's cleared on other 
insertions. If we make the framework candidates a function can we then do the 
clearing here as well? Or is it too expensive to compute on each recovery of 
resources?



src/master/allocator/mesos/hierarchical.cpp
Lines 1216 (patched)


Just curious, what would it mean if it were already in the set? Would it be 
a bug? Do you want to check for that?



src/master/allocator/mesos/hierarchical.cpp
Lines 2067-2087 (patched)


Can you contain this in a labmda?

```
hashmap roleFrameworkCandidates = [&]() {
  ...
  
  return candidates;
}();
```



src/master/allocator/mesos/hierarchical.cpp
Lines 2084 (patched)


std::move it into the map



src/master/allocator/mesos/hierarchical.cpp
Lines 2089-2107 (patched)


Do you actually need subset? The way this is used it seems like you just 
need to use equality?



src/master/allocator/mesos/hierarchical.cpp
Line 2061 (original)


The removal of this looks like an optimization to avoid re-lookup of the 
agent, so we could pull that out in front of this change to get it quickly 
committed and minimize this diff.



src/master/allocator/mesos/hierarchical.cpp
Lines 2145 (patched)


Can you use .at for read-only access here and elsewhere?



src/master/allocator/mesos/hierarchical.cpp
Lines 2154-2160 (patched)


> In this case, the agent will keep offering resources to the frameworks 
that are omitted in the exclusion set, starving others.

Hm.. is this right? It looks more like no one would be getting the offer? 
This invariant looks like: exclusion set for role != framework candidate set 
for role.

I guess the more difficult invariant to check is: how do we avoid getting 
"stuck" with a partially full exclusion set? In some cases, it seems like this 
is actually the implemented behavior: if a framework keeps accepting and 
lauching small very short-lived tasks on an agent, another framework that 
declined it might never get it back.



src/master/allocator/mesos/hierarchical.cpp
Lines 2169-2180 (patched)


Is this required for correctness or is this an optimization?



src/master/allocator/mesos/hierarchical.cpp
Lines 2188-2199 (patched)


Is this required for correctness or is this an optimization?



src/master/allocator/mesos/hierarchical.cpp
Lines 2104-2151 (original), 2230-2282 (patched)


Moving this up seems like an optimization 

Re: Review Request 66841: Added a hash function for 'Duration'.

[Greg Mann]

> On April 30, 2018, 11:57 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/duration.hpp
> > Lines 28 (patched)
> > 
> >
> > Instead of pulling in boost here, can we just leverage the built-in 
> > hash for int64_t?
> > 
> > http://en.cppreference.com/w/cpp/utility/hash
> > 
> > I guess this looks like:
> > 
> > ```
> > #include 
> > 
> > ...
> > 
> > namespace std {
> > 
> > template <>
> > struct hash
> > {
> >   typedef size_t result_type;
> >   typedef Duration argument_type;
> > 
> >   result_type operator()(const argument_type& d) const
> >   {
> > return hash{}(d.ns());
> >   }
> > };
> > 
> > } // namespace std {
> > ```

Good call, thanks Ben!


- Greg


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


On May 8, 2018, 11:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66841/
> ---
> 
> (Updated May 8, 2018, 11:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a hash function for 'Duration'.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/duration.hpp 
> 42c43cda21c75fc3bef962af67c4a09df68a95af 
> 
> 
> Diff: https://reviews.apache.org/r/66841/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 66635: Introduced wrapper for access to cgroups system access.

[Gilbert Song]

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


Fix it, then Ship it!




LGTM!


src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp
Lines 98 (patched)


Could we switch the order with `SubsystemProcess`? This is the semantic we 
follow in the agent code for practice. A forward declaration is needed.



src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 121-135 (patched)


ditto for the order in .cpp file.


- Gilbert Song


On May 8, 2018, 8:33 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> ---
> 
> (Updated May 8, 2018, 8:33 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 5763c9880728f02e44116fd50e62b592a8ef69b6 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4431ce13d28035b0c5c037b2848ae03aeaf65562 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9afa02b207e6272836e5a36d69fb48f1f4d02150 
> 
> 
> Diff: https://reviews.apache.org/r/66635/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66635: Introduced wrapper for access to cgroups system access.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On May 8, 2018, 11:33 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> ---
> 
> (Updated May 8, 2018, 11:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 5763c9880728f02e44116fd50e62b592a8ef69b6 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4431ce13d28035b0c5c037b2848ae03aeaf65562 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9afa02b207e6272836e5a36d69fb48f1f4d02150 
> 
> 
> Diff: https://reviews.apache.org/r/66635/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66997: Removed the OpenSSL dependency for building gRPC in libprocess.

[Chun-Hung Hsiao]

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

(Updated May 9, 2018, 12:52 a.m.)


Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description (updated)
---

When the SSL build feature is disabled, libprocess now builds
`libgrpc_unsecure` and `libgrpc++_unsecure` instead of `libgrpc` and
`libgrpc++`, so the SSL headers and libraries are no longer required.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/Makefile.am 
7fb77f8f29e00a276cfb0efc5324ac838c8b5176 
  3rdparty/libprocess/Makefile.am 70edb2ec3ad8b222cd28064f1012c39c335c57a5 
  3rdparty/libprocess/configure.ac fd65d7062ee42eccf2a68d23d02e456d07da6513 


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

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


Testing
---

sudo make check in Mesos build and libprocess standalone build, with openssl 
1.0.2l and 1.1.0f.


Thanks,

Chun-Hung Hsiao

Re: Review Request 66996: Removed the OpenSSL dependency for building gRPC in Mesos.

[Chun-Hung Hsiao]

> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Line 182 (original), 185 (patched)
> > 
> >
> > It is not clear to me that this would always find the correct 
> > libraries. I see e.g., that the cmake build does not seem to use the 
> > `_unsecure` suffix even when the libraries where built without SSL.
> > 
> > Do we need to explicitly discover these libraries during configure time 
> > to make sure we don't risk failing to link later?
> 
> Chun-Hung Hsiao wrote:
> We already did: 
> https://github.com/apache/mesos/blob/master/configure.ac#L2072. The 
> assumption here is that for custom grpc, we assume the whole grpc package is 
> installed so the standard grpc library must be there.
> 
> Chun-Hung Hsiao wrote:
> I'll just use the standard libs here as well.

Hmm. Actually, let me check the "unsecure" variant in `configure.ac` instead.


- Chun-Hung


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


On May 8, 2018, 3:23 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66996/
> ---
> 
> (Updated May 8, 2018, 3:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8798
> https://issues.apache.org/jira/browse/MESOS-8798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the SSL build feature is disabled, Mesos now builds
> `libgrpc_unsecure` and `libgrpc++_unsecure` instead of `libgrpc` and
> `libgrpc++`, so the SSL headers and libraries are no longer required.
> 
> NOTE: gRPC v1.10 no longer needs `-Wno-deprecated-declarations` and
> `-Wno-unused-function` when building with OpenSSL v1.1.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am 8d9fa85dd65a94d91670d54dab36deea80d14996 
>   configure.ac 429797c35b93a6df69ba0cb0fc28cb66a3171074 
>   src/Makefile.am 7e91681e3b8b217f8b23fa5347e059640c62fc65 
>   src/python/native_common/ext_modules.py.in 
> 87387fd580c40b252fc82f98b5238b9b9120903a 
> 
> 
> Diff: https://reviews.apache.org/r/66996/diff/1/
> 
> 
> Testing
> ---
> 
> This patch does not work standalone. Tests are done in the next patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 67022: Refreshed checkpointed SlaveInfo if `--reconfiguration_policy==any`.

[Zhitao Li]

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

Review request for mesos, Benno Evers, Jason Lai, and Vinod Kone.


Repository: mesos


Description
---

Refreshed checkpointed SlaveInfo if `--reconfiguration_policy==any`.


Diffs
-

  src/slave/slave.cpp c6d9152d9de4184f9107bb8242b41d468d76e018 


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


Testing
---


Thanks,

Zhitao Li

Review Request 67025: Fixed the link order for gRPC in CMake.

[Chun-Hung Hsiao]

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

Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.


Repository: mesos


Description
---

Fixed the link order for gRPC in CMake.


Diffs
-

  3rdparty/CMakeLists.txt 0075c133ef60bfef06f4123c14cae8a5672440f5 


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


Testing
---

sudo ninja check


Thanks,

Chun-Hung Hsiao

Review Request 67024: Renamed `GRPC_UNSECURE` to `GRPC_VARIANT` in CMake.

[Chun-Hung Hsiao]

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

Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.


Repository: mesos


Description
---

Renamed `GRPC_UNSECURE` to `GRPC_VARIANT` in CMake.


Diffs
-

  3rdparty/CMakeLists.txt 0075c133ef60bfef06f4123c14cae8a5672440f5 


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


Testing
---

sudo ninja check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67013: Sort container mounts by their target paths.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [67013, 67012, 65900, 65899, 65898, 65812, 65811]

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

Error:
2018-05-09 04:20:03 URL:https://reviews.apache.org/r/65899/diff/raw/ 
[11238/11238] -> "65899.patch" [1]
error: patch failed: src/slave/containerizer/mesos/launch.cpp:21
error: src/slave/containerizer/mesos/launch.cpp: patch does not apply

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

- Mesos Reviewbot


On May 8, 2018, 6:59 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67013/
> ---
> 
> (Updated May 8, 2018, 6:59 p.m.)
> 
> 
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, James Peach, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6798
> https://issues.apache.org/jira/browse/MESOS-6798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sort container mounts by their target paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch/linux_helper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67013/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 64384: Added new 'any' setting for reconfiguration_policy flag.

[Zhitao Li]

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




src/slave/slave.cpp
Lines 6373-6375 (original), 6377-6379 (patched)


I noticed that we do not refresh the checkpointed information of agent, but 
simply proceed to run a different `AgentInfo` from what's left on the disk.

Do we worry that this could cause a confusion in the future? I wonder 
whether we should augment the behavior for `any` to also flush out changed 
`AgentInfo` using `state::checkpoint()`.

Thoughts?


- Zhitao Li


On Dec. 6, 2017, 8:22 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64384/
> ---
> 
> (Updated Dec. 6, 2017, 8:22 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This setting allows any state change, effectively telling agents to ignore 
> the existing state and not to run any new tasks until the existing ones fit 
> into the new provided resources.
> 
> 
> Diffs
> -
> 
>   src/slave/compatibility.hpp 78b421a01abe5d2178c93832577577a7ba282b38 
>   src/slave/compatibility.cpp 4ead4a5b655f6f3d7812aa52d656830d7cff4598 
>   src/slave/flags.cpp d8764745e6aca81283d8b96388df1320c3465952 
>   src/slave/slave.cpp 49270013537356c8fe9150d757b064bc3bbae3cb 
> 
> 
> Diff: https://reviews.apache.org/r/64384/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 66996: Removed the OpenSSL dependency for building gRPC in Mesos.

[Chun-Hung Hsiao]

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

(Updated May 9, 2018, 12:50 a.m.)


Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description (updated)
---

When the SSL build feature is disabled, Mesos now builds
`libgrpc_unsecure` and `libgrpc++_unsecure` instead of `libgrpc` and
`libgrpc++`, so the SSL headers and libraries are no longer required.


Diffs (updated)
-

  3rdparty/Makefile.am 8d9fa85dd65a94d91670d54dab36deea80d14996 
  configure.ac 429797c35b93a6df69ba0cb0fc28cb66a3171074 
  src/Makefile.am 7e91681e3b8b217f8b23fa5347e059640c62fc65 
  src/python/native_common/ext_modules.py.in 
87387fd580c40b252fc82f98b5238b9b9120903a 


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

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


Testing
---

This patch does not work standalone. Tests are done in the next patch.


Thanks,

Chun-Hung Hsiao

Review Request 67021: Windows: Enabled `ProtobufTest` suite.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and 
Radhika Jandhyala.


Repository: mesos


Description
---

These tests just worked once added to the build. Also, out-of-date
code was deleted from `systems_tests.cpp`, and a note added to the
build file that it (and `signals_tests.cpp`) will be not be ported to
Windows.


Diffs
-

  3rdparty/stout/tests/CMakeLists.txt 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
  3rdparty/stout/tests/os/systems_tests.cpp 
a2a05ce28d11cdb97a511e5392a05522a45826c8 


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


Testing
---

I just noticed that these weren't enabled when checking something else. Turned 
them on, they passed. Will push when CI passes.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 66882: Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`.

[Gaston Kleiman]

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

(Updated May 8, 2018, 5:06 p.m.)


Review request for mesos, Benjamin Mahler, Gilbert Song, Greg Mann, and Vinod 
Kone.


Summary (updated)
-

Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`.


Repository: mesos


Description (updated)
---

Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`.


Diffs (updated)
-

  src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
  src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 


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

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


Testing
---


Thanks,

Gaston Kleiman

Re: Review Request 67022: Refreshed checkpointed SlaveInfo if `--reconfiguration_policy==any`.

[Zhitao Li]

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

(Updated May 8, 2018, 5:40 p.m.)


Review request for mesos, Eric Chung and Jason Lai.


Repository: mesos


Description
---

Refreshed checkpointed SlaveInfo if `--reconfiguration_policy==any`.


Diffs (updated)
-

  src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
  src/slave/slave.cpp c6d9152d9de4184f9107bb8242b41d468d76e018 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 67022: Refreshed checkpointed SlaveInfo if `--reconfiguration_policy==any`.

[Jason Lai]

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


Ship it!




Ship It!

- Jason Lai


On May 9, 2018, 12:40 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67022/
> ---
> 
> (Updated May 9, 2018, 12:40 a.m.)
> 
> 
> Review request for mesos, Eric Chung and Jason Lai.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refreshed checkpointed SlaveInfo if `--reconfiguration_policy==any`.
> 
> 
> Diffs
> -
> 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/slave/slave.cpp c6d9152d9de4184f9107bb8242b41d468d76e018 
> 
> 
> Diff: https://reviews.apache.org/r/67022/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66934: Added token-based authentication to resource provider tests.

[Jan Schlicht]

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

(Updated May 8, 2018, 11:38 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

If compiled with SSL support, resource provider tests will use
authentication. The 'MockResourceProvider' has been updated to create a
JWT when started.


Diffs (updated)
-

  src/tests/agent_resource_provider_config_api_tests.cpp 
6ced582e14966eea3bb109e2e2de0e554acc2968 
  src/tests/api_tests.cpp 1ed26a7d796be3e3d0a44d107d2f769a39ff450a 
  src/tests/executor_http_api_tests.cpp 
51cca7a90b3c5c5380ede0f17d66b9fdfeb88c27 
  src/tests/files_tests.cpp 4286e58d3407a2b82cf720651697ee635b10ad63 
  src/tests/logging_tests.cpp 886dcd0d6d6ccf509c98578e8b86b1c6ecd5775f 
  src/tests/master_slave_reconciliation_tests.cpp 
937bab08c9bef1a2a6a400979dcf0895412168f5 
  src/tests/master_tests.cpp 0f6042ca15ac5862d5dc73f113bbe23504eda65c 
  src/tests/mesos.hpp b945edf07e98422bf6bfceb6d0d9fa2ff188aece 
  src/tests/metrics_tests.cpp 55065cd11ece50e5b79a8e72a4075ebc93817794 
  src/tests/operation_reconciliation_tests.cpp 
39cf1888ed1ad9395203c6f9c6ffd87fc7623195 
  src/tests/resource_provider_manager_tests.cpp 
c2045f2ba24f3e4b959115f23b706e733a75fea8 
  src/tests/slave_tests.cpp 9e398842e57fda67aaed9fd23e15eef4ce180e05 
  src/tests/storage_local_resource_provider_tests.cpp 
ccb114aacc904998dfeef4128f6d38dfb3c865c7 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 66997: Removed the OpenSSL dependency for building gRPC in libprocess.

[Benjamin Bannier]

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




3rdparty/libprocess/3rdparty/Makefile.am
Lines 208 (patched)


This variable name makes sense only 50% of the time, how about e.g., 
`GRPC_LIB_SUFFIX`?



3rdparty/libprocess/3rdparty/Makefile.am
Lines 223-232 (original), 221-230 (patched)


It is very hard to see what is actually being execute here in what 
environment. Could you reflow this to e.g., always pass the env after `make`, 
reorder the variables and reflow the code?

$(MAKE) $(AM_MAKEFLAGS) \
  $(LIB_GRPC:%=$(abs_builddir)/%)   \
  CC="$(CC)"\
  CXX="$(CXX)"  \
  LD="$(CC)"\
  LDXX="$(CXX)" \
  CPPFLAGS="$(PROTOBUF_INCLUDE_FLAGS)   \
$(SSL_INCLUDE_FLAGS)\
$(ZLIB_INCLUDE_FLAGS)"  \
  LDFLAGS="$(PROTOBUF_LINKER_FLAGS) \
   $(SSL_LINKER_FLAGS)  \
   $(ZLIB_LINKER_FLAGS)"\
  HAS_PKG_CONFIG=false  \
  NO_PROTOC=false   \
  PROTOC="$(PROTOC)"



3rdparty/libprocess/3rdparty/Makefile.am
Lines 223-224 (original), 221-222 (patched)


Why are we removing these extra flags? They don't seem to come from the 
normal flags.


- Benjamin Bannier


On May 8, 2018, 5:25 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66997/
> ---
> 
> (Updated May 8, 2018, 5:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8798
> https://issues.apache.org/jira/browse/MESOS-8798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the SSL build feature is disabled, libprocess now builds
> `libgrpc_unsecure` and `libgrpc++_unsecure` instead of `libgrpc` and
> `libgrpc++`, so the SSL headers and libraries are no longer required.
> 
> NOTE: gRPC v1.10 no longer needs `-Wno-deprecated-declarations` and
> `-Wno-unused-function` when building with OpenSSL v1.1.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 7fb77f8f29e00a276cfb0efc5324ac838c8b5176 
>   3rdparty/libprocess/Makefile.am 70edb2ec3ad8b222cd28064f1012c39c335c57a5 
>   3rdparty/libprocess/configure.ac fd65d7062ee42eccf2a68d23d02e456d07da6513 
> 
> 
> Diff: https://reviews.apache.org/r/66997/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check in Mesos build and libprocess standalone build, with openssl 
> 1.0.2l and 1.1.0f.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66996: Removed the OpenSSL dependency for building gRPC in Mesos.

[Benjamin Bannier]

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




3rdparty/Makefile.am
Lines 326 (patched)


This variable name makes sense only 50% of the time, how about e.g., 
`GRPC_LIB_SUFFIX`?



3rdparty/Makefile.am
Lines 341-350 (original), 339-348 (patched)


It is very hard to see what is actually being execute here in what 
environment. Could you reflow this to e.g., always pass the env after `make`, 
reorder the variables and reflow the code?

$(MAKE) $(AM_MAKEFLAGS) \
  $(LIB_GRPC:%=$(abs_builddir)/%)   \
  CC="$(CC)"\
  CXX="$(CXX)"  \
  LD="$(CC)"\
  LDXX="$(CXX)" \
  CPPFLAGS="$(PROTOBUF_INCLUDE_FLAGS)   \
$(SSL_INCLUDE_FLAGS)\
$(ZLIB_INCLUDE_FLAGS)"  \
  LDFLAGS="$(PROTOBUF_LINKER_FLAGS) \
   $(SSL_LINKER_FLAGS)  \
   $(ZLIB_LINKER_FLAGS)"\
  HAS_PKG_CONFIG=false  \
  NO_PROTOC=false   \
  PROTOC="$(PROTOC)"



3rdparty/Makefile.am
Lines 341-342 (original), 339-340 (patched)


Why are we removing these extra flags? They don't seem to come from the 
normal flags?



src/Makefile.am
Lines 175 (patched)


Why are we removing these extra flags? They don't seem to come from the 
normal flags?



src/Makefile.am
Line 182 (original), 185 (patched)


It is not clear to me that this would always find the correct libraries. I 
see e.g., that the cmake build does not seem to use the `_unsecure` suffix even 
when the libraries where built without SSL.

Do we need to explicitly discover these libraries during configure time to 
make sure we don't risk failing to link later?



src/python/native_common/ext_modules.py.in
Line 132 (original), 132 (patched)


This variable name makes sense only 50% of the time, how about e.g., 
`grpc_lib_suffix`?



src/python/native_common/ext_modules.py.in
Lines 151-152 (original)


How do we manage to link an object file (which has no dependency 
information) when SSL is enabled?


- Benjamin Bannier


On May 8, 2018, 5:23 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66996/
> ---
> 
> (Updated May 8, 2018, 5:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8798
> https://issues.apache.org/jira/browse/MESOS-8798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the SSL build feature is disabled, Mesos now builds
> `libgrpc_unsecure` and `libgrpc++_unsecure` instead of `libgrpc` and
> `libgrpc++`, so the SSL headers and libraries are no longer required.
> 
> NOTE: gRPC v1.10 no longer needs `-Wno-deprecated-declarations` and
> `-Wno-unused-function` when building with OpenSSL v1.1.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am 8d9fa85dd65a94d91670d54dab36deea80d14996 
>   configure.ac 429797c35b93a6df69ba0cb0fc28cb66a3171074 
>   src/Makefile.am 7e91681e3b8b217f8b23fa5347e059640c62fc65 
>   src/python/native_common/ext_modules.py.in 
> 87387fd580c40b252fc82f98b5238b9b9120903a 
> 
> 
> Diff: https://reviews.apache.org/r/66996/diff/1/
> 
> 
> Testing
> ---
> 
> This patch does not work standalone. Tests are done in the next patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66987: Renamed cgroups subsystem processes.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On May 7, 2018, 11:56 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66987/
> ---
> 
> (Updated May 7, 2018, 11:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Qian Zhang.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The different cgroups subsystems are modelled as actors implemented as
> libprocess `Process`es. This patch renames the classes to clearly
> reflect their `Process` nature.
> 
> In a subsequent patch we will introduce a wrapper class to access
> subsystems.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 5763c9880728f02e44116fd50e62b592a8ef69b6 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4431ce13d28035b0c5c037b2848ae03aeaf65562 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9afa02b207e6272836e5a36d69fb48f1f4d02150 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
> ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
> cba21ede59916b0a6e120ecd2b6348b8a946c507 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
> 282a76189579d3ddd61f4aad210ce8e876ff0c25 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 75fab0f1fc7e7403855de0786b8c1155d7599b7f 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> e4977da476a04a708a2e9b9dcc5a2f85da5867f4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> 3be419ad477e9baa329b5388d4c12fa743c7f563 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.hpp 
> b5d712a8eb8ba74092184024e3b40ad9ba1b7584 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.cpp 
> 328ebc63933a014aa00f68ad49243e99cb60e21e 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
> 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> 4c43191251d7411d14ed8b11c79a64ff5d6ccd76 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.hpp 
> 27407133e7315dccf1efe2440cc1bf79c51e7dca 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.cpp 
> 0af4d2eacebc0d7800aabfad92485636e60ce2dd 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> d79fda3cf852e50e75dbeeeaeae50b1f471a55a4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> 50348d63f6fffa7662e6b91b5ce4ff730380e708 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
> bcaa16142c4e9882dea88e70095cfb7f223401ef 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
> f778a419e15940d92079d04884887615322791f5 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.hpp 
> 002c689503d45622cba437c851561f5ec7dc8a12 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.cpp 
> 060556bd48fed9b70d6994b9d4debc9c1c5098a7 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
> 14c0e79f045fb826d20e476772e017439977dded 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
> d394d793c6bc38e249530c54ddb868ecff7f7865 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/pids.hpp 
> cb6c91920355d3d5599f8a3cf0ce2ac5eee18d37 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/pids.cpp 
> 42fea606000dea23a64b43252d8560ef2a192892 
> 
> 
> Diff: https://reviews.apache.org/r/66987/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66933: Added a realm for resource provider authentication.

[Jan Schlicht]

> On May 8, 2018, 12:35 p.m., Benjamin Bannier wrote:
> > src/slave/constants.hpp
> > Lines 171 (patched)
> > 
> >
> > I don't believe it makes sense to treat resource providers separately 
> > from other internal components using HTTP communication, what about e.g.,
> > 
> > // Name of the agent HTTP authentication realm for internal Mesos 
> > components.
> > constexpr char INTERNAL_HTTP_AUTHENTICATION_REALM[] = 
> > "mesos-internal";

While the resource provider API currently is only used internally, it's 
expected to be used externally as well. If it would be internal only, we would 
communicate using actors.


- Jan


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


On May 7, 2018, 2:12 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66933/
> ---
> 
> (Updated May 7, 2018, 2:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8774
> https://issues.apache.org/jira/browse/MESOS-8774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp d1d15c39c87b9712c96b9f7516a7a0a3e56514cf 
>   src/slave/slave.cpp c6d9152d9de4184f9107bb8242b41d468d76e018 
>   src/tests/cluster.cpp c071da69500e1d8a223f255904acf7e28100e774 
> 
> 
> Diff: https://reviews.apache.org/r/66933/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63367: Added overloads for strings::startsWith and strings::endsWith().

[Benno Evers]

> On May 3, 2018, 9:35 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/strings.hpp
> > Lines 390 (patched)
> > 
> >
> > Can you mention the max length optimization you're doing?

I will add a comment if you think it is non-obvious, but intuitively I would 
have said that this is no more complicated than the other algorithms in this 
file?


> On May 3, 2018, 9:35 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/strings.hpp
> > Lines 411-413 (patched)
> > 
> >
> > Any reason not to do the same max len optimization here?

There was, but as I was typing this reply I realized it not actually valid, so 
I'll update the review to add it as well.


- Benno


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


On May 8, 2018, 12:30 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63367/
> ---
> 
> (Updated May 8, 2018, 12:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This saves an unnecessary memory allocation when
> testing if a string starts or ends with a string literal,
> which accounts for almost all usages of these functions
> in Mesos and in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/strings.hpp 
> 067a7923c02342bccd9be1136a981fd6b0e0e9b4 
>   3rdparty/stout/tests/strings_tests.cpp 
> 395540aad88c76a66a43a54edfe9ca1a2d46d3b4 
> 
> 
> Diff: https://reviews.apache.org/r/63367/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63367: Added overloads for strings::startsWith and strings::endsWith().

[Benno Evers]

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

(Updated May 8, 2018, 12:30 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Repository: mesos


Description
---

This saves an unnecessary memory allocation when
testing if a string starts or ends with a string literal,
which accounts for almost all usages of these functions
in Mesos and in libprocess.


Diffs (updated)
-

  3rdparty/stout/include/stout/strings.hpp 
067a7923c02342bccd9be1136a981fd6b0e0e9b4 
  3rdparty/stout/tests/strings_tests.cpp 
395540aad88c76a66a43a54edfe9ca1a2d46d3b4 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 66933: Added a realm for resource provider authentication.

[Benjamin Bannier]

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




src/slave/constants.hpp
Lines 171 (patched)


I don't believe it makes sense to treat resource providers separately from 
other internal components using HTTP communication, what about e.g.,

// Name of the agent HTTP authentication realm for internal Mesos 
components.
constexpr char INTERNAL_HTTP_AUTHENTICATION_REALM[] = "mesos-internal";


- Benjamin Bannier


On May 7, 2018, 2:12 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66933/
> ---
> 
> (Updated May 7, 2018, 2:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8774
> https://issues.apache.org/jira/browse/MESOS-8774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp d1d15c39c87b9712c96b9f7516a7a0a3e56514cf 
>   src/slave/slave.cpp c6d9152d9de4184f9107bb8242b41d468d76e018 
>   src/tests/cluster.cpp c071da69500e1d8a223f255904acf7e28100e774 
> 
> 
> Diff: https://reviews.apache.org/r/66933/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 66932: Adden token based authentication for resource providers.

[Benjamin Bannier]

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


Fix it, then Ship it!





include/mesos/v1/resource_provider.hpp
Line 83 (original), 83 (patched)


Could you update the docstring to include `token` both in the description 
text and the param list? it would be great to also document the expected 
semantics of the passed `string`.


- Benjamin Bannier


On May 3, 2018, 2:55 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66932/
> ---
> 
> (Updated May 3, 2018, 2:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8774
> https://issues.apache.org/jira/browse/MESOS-8774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a token is provided, it will be used in HTTP requests to the resource
> provider manager. This allows JWT based authentication and authorization
> for resource providers.
> The (unimplemented) credential support in `resource_provider::Driver`
> has been removed in favor of the token-based approach.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/resource_provider.hpp 
> 787c619cb6d7574c3ffcd22c5ad2f3a6a3b56694 
>   src/resource_provider/driver.cpp 70b7e3244a18dd854a85c65853fe32d82a5ec7d4 
>   src/resource_provider/http_connection.hpp 
> 0ae8124150dff96296a4a2c358338fe196717b1a 
>   src/resource_provider/storage/provider.cpp 
> d1267cf8df06cc758c47d47535b868a3ac741a0b 
>   src/tests/mesos.hpp 756a52189ea3336b203ea1d606e2ba17762d57aa 
> 
> 
> Diff: https://reviews.apache.org/r/66932/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 66934: Added token-based authentication to resource provider tests.

[Benjamin Bannier]

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


Fix it, then Ship it!




Could you verify that after rebase no tests set 
`SlaveFlags::authenticate_http_readwrite = false` when not needed?


src/tests/files_tests.cpp
Line 72 (original), 70 (patched)


Duh, this is one argument why using decls do not scale 
(`process::http::authentication::Authenticator` vs `mesos::Authenticator`).



src/tests/files_tests.cpp
Line 81 (original), 79 (patched)


To keep with the current style lets add using decls for `setAuthenticator` 
and `unsetAuthenticator`.



src/tests/logging_tests.cpp
Line 65 (original), 63 (patched)


Add using decl.



src/tests/logging_tests.cpp
Line 75 (original), 74 (patched)


Add using decl.



src/tests/master_slave_reconciliation_tests.cpp
Lines 449-450 (original), 449-450 (patched)


This is not needed anymore.



src/tests/mesos.hpp
Lines 72 (patched)


Can we include this unconditionally?



src/tests/mesos.hpp
Lines 3084-3085 (patched)


Let's add a `TODO` to revisit this should we add authorization of claims.



src/tests/metrics_tests.cpp
Line 68 (original), 66 (patched)


Add using decl.



src/tests/metrics_tests.cpp
Line 78 (original), 77 (patched)


Add using decl.


- Benjamin Bannier


On May 8, 2018, 11:38 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66934/
> ---
> 
> (Updated May 8, 2018, 11:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8774
> https://issues.apache.org/jira/browse/MESOS-8774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If compiled with SSL support, resource provider tests will use
> authentication. The 'MockResourceProvider' has been updated to create a
> JWT when started.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_resource_provider_config_api_tests.cpp 
> 6ced582e14966eea3bb109e2e2de0e554acc2968 
>   src/tests/api_tests.cpp 1ed26a7d796be3e3d0a44d107d2f769a39ff450a 
>   src/tests/executor_http_api_tests.cpp 
> 51cca7a90b3c5c5380ede0f17d66b9fdfeb88c27 
>   src/tests/files_tests.cpp 4286e58d3407a2b82cf720651697ee635b10ad63 
>   src/tests/logging_tests.cpp 886dcd0d6d6ccf509c98578e8b86b1c6ecd5775f 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 937bab08c9bef1a2a6a400979dcf0895412168f5 
>   src/tests/master_tests.cpp 0f6042ca15ac5862d5dc73f113bbe23504eda65c 
>   src/tests/mesos.hpp b945edf07e98422bf6bfceb6d0d9fa2ff188aece 
>   src/tests/metrics_tests.cpp 55065cd11ece50e5b79a8e72a4075ebc93817794 
>   src/tests/operation_reconciliation_tests.cpp 
> 39cf1888ed1ad9395203c6f9c6ffd87fc7623195 
>   src/tests/resource_provider_manager_tests.cpp 
> c2045f2ba24f3e4b959115f23b706e733a75fea8 
>   src/tests/slave_tests.cpp 9e398842e57fda67aaed9fd23e15eef4ce180e05 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ccb114aacc904998dfeef4128f6d38dfb3c865c7 
> 
> 
> Diff: https://reviews.apache.org/r/66934/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 67004: Added an option to keep downloaded patches to apply-reviews.py.

[Armand Grillet]

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


Ship it!




Ship It!

- Armand Grillet


On May 8, 2018, 12:54 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67004/
> ---
> 
> (Updated May 8, 2018, 12:54 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By default, the apply-reviews.py script will delete all patch files it 
> downloads. However, when a patch fails to apply, it might be desired to edit 
> and apply it manually. This change will make it easier to do so.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py b6801b926d9fc32172f102586d49e0464da71359 
> 
> 
> Diff: https://reviews.apache.org/r/67004/diff/1/
> 
> 
> Testing
> ---
> 
> Ran `./support/apply-reviews.py` script with and without the new option.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 66932: Adden token based authentication for resource providers.

[Jan Schlicht]

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

(Updated May 8, 2018, 3:08 p.m.)


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


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

If a token is provided, it will be used in HTTP requests to the resource
provider manager. This allows JWT based authentication and authorization
for resource providers.
The (unimplemented) credential support in `resource_provider::Driver`
has been removed in favor of the token-based approach.


Diffs (updated)
-

  include/mesos/v1/resource_provider.hpp 
787c619cb6d7574c3ffcd22c5ad2f3a6a3b56694 
  src/resource_provider/driver.cpp 70b7e3244a18dd854a85c65853fe32d82a5ec7d4 
  src/resource_provider/http_connection.hpp 
0ae8124150dff96296a4a2c358338fe196717b1a 
  src/resource_provider/storage/provider.cpp 
eb35780d96b58b229766c94f87000cd9821d10e5 
  src/tests/mesos.hpp b945edf07e98422bf6bfceb6d0d9fa2ff188aece 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 66997: Removed the OpenSSL dependency for building gRPC in libprocess.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66996, 66997]

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

- Mesos Reviewbot


On May 8, 2018, 11:25 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66997/
> ---
> 
> (Updated May 8, 2018, 11:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8798
> https://issues.apache.org/jira/browse/MESOS-8798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the SSL build feature is disabled, libprocess now builds
> `libgrpc_unsecure` and `libgrpc++_unsecure` instead of `libgrpc` and
> `libgrpc++`, so the SSL headers and libraries are no longer required.
> 
> NOTE: gRPC v1.10 no longer needs `-Wno-deprecated-declarations` and
> `-Wno-unused-function` when building with OpenSSL v1.1.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 7fb77f8f29e00a276cfb0efc5324ac838c8b5176 
>   3rdparty/libprocess/Makefile.am 70edb2ec3ad8b222cd28064f1012c39c335c57a5 
>   3rdparty/libprocess/configure.ac fd65d7062ee42eccf2a68d23d02e456d07da6513 
> 
> 
> Diff: https://reviews.apache.org/r/66997/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check in Mesos build and libprocess standalone build, with openssl 
> 1.0.2l and 1.1.0f.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 67003: Fixed flakiness in a `MasterSlaveReconciliationTest`.

[Benjamin Bannier]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

The test `ReconcileDroppedOperation` uses detection of a
`ReconcileOperationsMessage` to confirm correct agent reregistration
behavior. For that it drops an operation on its way to the agent, and
then tries to observe the `ReconcileOperationsMessage` when the agent
reregisters after a simulated master failover.

Since `ReconcileOperationsMessage` is sent whenever the master detects
discrepancy between its own operation state of the agent and the
information sent by the agent in an `UpdateSlaveMessage` we need to
make sure to only drop the operation once the agent has sent the
update which is part of its initial registration sequence.


Diffs
-

  src/tests/master_slave_reconciliation_tests.cpp 
937bab08c9bef1a2a6a400979dcf0895412168f5 


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


Testing
---

`make check`

Ran the test in repetition under heavy (simulated) load for several thousand 
iterations without failing; before this patch it would fail after a couple 
dozen iterations.


Thanks,

Benjamin Bannier

Review Request 67004: Added an option to keep downloaded patches to apply-reviews.py.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

By default, the apply-reviews.py script will delete all patch files it 
downloads. However, when a patch fails to apply, it might be desired to edit 
and apply it manually. This change will make it easier to do so.


Diffs
-

  support/apply-reviews.py b6801b926d9fc32172f102586d49e0464da71359 


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


Testing
---

Ran `./support/apply-reviews.py` script with and without the new option.


Thanks,

Benno Evers

Re: Review Request 66934: Added token-based authentication to resource provider tests.

[Jan Schlicht]

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

(Updated May 8, 2018, 3:09 p.m.)


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


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

If compiled with SSL support, resource provider tests will use
authentication. The 'MockResourceProvider' has been updated to create a
JWT when started.


Diffs (updated)
-

  src/tests/agent_resource_provider_config_api_tests.cpp 
6ced582e14966eea3bb109e2e2de0e554acc2968 
  src/tests/api_tests.cpp 1ed26a7d796be3e3d0a44d107d2f769a39ff450a 
  src/tests/executor_http_api_tests.cpp 
51cca7a90b3c5c5380ede0f17d66b9fdfeb88c27 
  src/tests/files_tests.cpp 4286e58d3407a2b82cf720651697ee635b10ad63 
  src/tests/logging_tests.cpp 886dcd0d6d6ccf509c98578e8b86b1c6ecd5775f 
  src/tests/master_slave_reconciliation_tests.cpp 
937bab08c9bef1a2a6a400979dcf0895412168f5 
  src/tests/master_tests.cpp 0f6042ca15ac5862d5dc73f113bbe23504eda65c 
  src/tests/mesos.hpp b945edf07e98422bf6bfceb6d0d9fa2ff188aece 
  src/tests/metrics_tests.cpp 55065cd11ece50e5b79a8e72a4075ebc93817794 
  src/tests/operation_reconciliation_tests.cpp 
39cf1888ed1ad9395203c6f9c6ffd87fc7623195 
  src/tests/resource_provider_manager_tests.cpp 
c2045f2ba24f3e4b959115f23b706e733a75fea8 
  src/tests/slave_tests.cpp 9e398842e57fda67aaed9fd23e15eef4ce180e05 
  src/tests/storage_local_resource_provider_tests.cpp 
ccb114aacc904998dfeef4128f6d38dfb3c865c7 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 66635: Introduced wrapper for access to cgroups system access.

[Benjamin Bannier]

> On May 8, 2018, 5 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp
> > Line 174 (original), 236 (patched)
> > 
> >
> > It seems `subsystemName` is only used in `Subsystem::name()`, so 
> > instead of 
> > introducing this member variable, can we just call `process->name()` in 
> > `Subsystem::name()`?

I went this way because we do not really have a clear pattern on how to safely 
mark wrapped functions which are safe to access concurrently. I think it makes 
sense to expose the function directly (as opposed to accessing it through a 
`dispatch`) since the function itself is `const`, so I removed `subsystemName`.


> On May 8, 2018, 5 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
> > Lines 122 (patched)
> > 
> >
> > Mind to explain why we need `std::move` here? Want to leverage move 
> > constructor?

Semantically an `Owned` cannot be copied since it models unique ownership. 
MESOS-5122 should address making it impossible to copy instances, but until it 
is fixed we need to use `Owned` carefully and manually `move` instead of 
copying it implictly.


> On May 8, 2018, 5 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
> > Lines 136 (patched)
> > 
> >
> > What about just making this method's return type as `Try` 
> > and then `return new Subsystem(subsystemProcess.get())` at the end of this 
> > method?

What would the benefit of that be? It would require manual memory management I 
believe we should avoid since we have a library (`Owned`) doing it for us.

Please feel free to reopen if I am missing something.


- Benjamin


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


On May 8, 2018, 5:33 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> ---
> 
> (Updated May 8, 2018, 5:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 5763c9880728f02e44116fd50e62b592a8ef69b6 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4431ce13d28035b0c5c037b2848ae03aeaf65562 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9afa02b207e6272836e5a36d69fb48f1f4d02150 
> 
> 
> Diff: https://reviews.apache.org/r/66635/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 67008: Added documentation to website for Python components.

[Armand Grillet]

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

Review request for mesos, Andrew Schwartzmeyer, Eric Chung, and Kevin Klues.


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


Repository: mesos


Description
---

Added documentation to website for Python components.


Diffs
-

  docs/home.md 5471c70d4023f88adae2234aec267e4d6fea83df 
  docs/python.md PRE-CREATION 


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


Testing
---


Thanks,

Armand Grillet

Re: Review Request 67003: Fixed flakiness in a `MasterSlaveReconciliationTest`.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On May 8, 2018, 11:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67003/
> ---
> 
> (Updated May 8, 2018, 11:59 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8892
> https://issues.apache.org/jira/browse/MESOS-8892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test `ReconcileDroppedOperation` uses detection of a
> `ReconcileOperationsMessage` to confirm correct agent reregistration
> behavior. For that it drops an operation on its way to the agent, and
> then tries to observe the `ReconcileOperationsMessage` when the agent
> reregisters after a simulated master failover.
> 
> Since `ReconcileOperationsMessage` is sent whenever the master detects
> discrepancy between its own operation state of the agent and the
> information sent by the agent in an `UpdateSlaveMessage` we need to
> make sure to only drop the operation once the agent has sent the
> update which is part of its initial registration sequence.
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 937bab08c9bef1a2a6a400979dcf0895412168f5 
> 
> 
> Diff: https://reviews.apache.org/r/67003/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Ran the test in repetition under heavy (simulated) load for several thousand 
> iterations without failing; before this patch it would fail after a couple 
> dozen iterations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66635: Introduced wrapper for access to cgroups system access.

[Benjamin Bannier]

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

(Updated May 8, 2018, 5:33 p.m.)


Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


Changes
---

Addressed issues raised by Qian.


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


Repository: mesos


Description
---

Different cgroups subsystems are modelled as actors. In this patch we
introduce wrapper classes which `dispatch` to the processes. This
removes e.g., races from mixing naked and `dispatch`'ed method calls.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
5763c9880728f02e44116fd50e62b592a8ef69b6 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4431ce13d28035b0c5c037b2848ae03aeaf65562 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
9afa02b207e6272836e5a36d69fb48f1f4d02150 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66635: Introduced wrapper for access to cgroups system access.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp
Line 174 (original), 236 (patched)


It seems `subsystemName` is only used in `Subsystem::name()`, so instead of 
introducing this member variable, can we just call `process->name()` in 
`Subsystem::name()`?



src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 121 (patched)


No need `process::`?



src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 122 (patched)


Mind to explain why we need `std::move` here? Want to leverage move 
constructor?



src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 136 (patched)


What about just making this method's return type as `Try` and 
then `return new Subsystem(subsystemProcess.get())` at the end of this method?



src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Line 83 (original), 173 (patched)


No need `process::`? Ditto for all other places in this file.



src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Line 92 (original), 175 (patched)


No need `std::`? Ditto for all other places in this file.



src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Line 108 (original), 199 (patched)


These two parameters should be in two lines.


- Qian Zhang


On May 7, 2018, 11:56 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> ---
> 
> (Updated May 7, 2018, 11:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 5763c9880728f02e44116fd50e62b592a8ef69b6 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4431ce13d28035b0c5c037b2848ae03aeaf65562 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 65c1e47a569f320b63b54e5f4fc1da374d02ee0d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9afa02b207e6272836e5a36d69fb48f1f4d02150 
> 
> 
> Diff: https://reviews.apache.org/r/66635/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 67009: Added tests of resource provider registrar recovery.

[Benjamin Bannier]

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

Review request for mesos and Chun-Hung Hsiao.


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


Repository: mesos


Description
---

Added tests of resource provider registrar recovery.


Diffs
-

  src/tests/resource_provider_manager_tests.cpp 
c2045f2ba24f3e4b959115f23b706e733a75fea8 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66993: Removed unnecessary expectation on termination.

[Andrei Budnik]

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




src/tests/containerizer/docker_containerizer_tests.cpp
Line 644 (original), 644 (patched)


We can add
```cpp
  Future executorTerminated =
FUTURE_DISPATCH(_, ::executorTerminated);
```
here and wait for it before stopping the driver
```cpp
AWAIT_READY(executorTerminated);
```
WDYT?


- Andrei Budnik


On May 7, 2018, 10:25 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66993/
> ---
> 
> (Updated May 7, 2018, 10:25 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and James Peach.
> 
> 
> Bugs: MESOS-8884
> https://issues.apache.org/jira/browse/MESOS-8884
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test was flaky because termination could already happened when we
> set up the expectation. Given that we already verified task state, I do
> not see checking container termination explicitly is necessary, so
> removing the expectation should fix the flakiness.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> d834e531a550028cd57ac409c9312dd22138e8d5 
> 
> 
> Diff: https://reviews.apache.org/r/66993/diff/1/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="DockerContainerizerTest.ROOT_DOCKER_MaxCompletionTime" 
> ./bin/mesos-tests.sh --gtest_repeat=100 --verbose
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66984: Added the ability to exhaustively allocate declined resources.

[Meng Zhu]

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

(Updated May 8, 2018, 9:36 a.m.)


Review request for mesos, Benjamin Mahler, Kapil Arya, Till Toenshoff, and 
Vinod Kone.


Changes
---

Patch updated:
- Made exclusion set role aware;
- Moved exclusion set to slave struct;
- Added invariant check; 
- Added a todo in quota allocation stage;
- Documented a design decision in the description.


Repository: mesos


Description (updated)
---

Schedulers that are below their fair share will continue to get
allocated resources even if they don't need those resources. The
expected behavior here is that schedulers will decline those resources
for a long time (or forever). Not all schedulers do this, however,
which means that some schedulers might get _starved_ of
resources. Technically these schedulers are already at or above their
fair share, but some operators feel that this is keeping the cluster
underutilized.

Rather than guarantee that all schedulers will decline with large
timeouts this patch ensures that all other schedulers will see
resources from the declined agent before they see resources from that
agent again, even if there are now more resources available on that
agent.

To this end, each agent maintains an exclusion set of frameworks
that declined its allocation earlier and skips them during the
allocation. Note, a framework here is associated with each of
its roles. If a framework declined resources allocated to one
of its roles, it can still get allocations for its other roles.

Note, an agent's exclusion set will be cleared if its attribute
changes.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
955ae3e6a9e3c790fb260311ec4b4ef725a826d3 
  src/master/allocator/mesos/hierarchical.cpp 
1000968be6a2935a4cac571414d7f06d7df7acf0 


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

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


Testing
---

make check
Dedicated test in #66994


Thanks,

Meng Zhu

Re: Review Request 67003: Fixed flakiness in a `MasterSlaveReconciliationTest`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67003]

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

- Mesos Reviewbot


On May 8, 2018, 11:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67003/
> ---
> 
> (Updated May 8, 2018, 11:59 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8892
> https://issues.apache.org/jira/browse/MESOS-8892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test `ReconcileDroppedOperation` uses detection of a
> `ReconcileOperationsMessage` to confirm correct agent reregistration
> behavior. For that it drops an operation on its way to the agent, and
> then tries to observe the `ReconcileOperationsMessage` when the agent
> reregisters after a simulated master failover.
> 
> Since `ReconcileOperationsMessage` is sent whenever the master detects
> discrepancy between its own operation state of the agent and the
> information sent by the agent in an `UpdateSlaveMessage` we need to
> make sure to only drop the operation once the agent has sent the
> update which is part of its initial registration sequence.
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 937bab08c9bef1a2a6a400979dcf0895412168f5 
> 
> 
> Diff: https://reviews.apache.org/r/67003/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Ran the test in repetition under heavy (simulated) load for several thousand 
> iterations without failing; before this patch it would fail after a couple 
> dozen iterations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66819: Added FrameworkMetrics struct in framework struct.

[Gaston Kleiman]

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




src/master/metrics.hpp
Lines 219-221 (patched)


This constructor should be implemented in this patch.

The implementation is introduced in https://reviews.apache.org/r/66820/ 
with extra parameters.


- Gaston Kleiman


On April 27, 2018, 11:39 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66819/
> ---
> 
> (Updated April 27, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added FrameworkMetrics struct in framework struct.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
> 
> 
> Diff: https://reviews.apache.org/r/66819/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66799: Fixed flakyness in 'MasterAPITest.MasterFailover'.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On April 25, 2018, 4:46 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66799/
> ---
> 
> (Updated April 25, 2018, 4:46 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8687
> https://issues.apache.org/jira/browse/MESOS-8687
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test used to be sporadically segfault as described in MESOS-8687.
> The suspected cause is that a in a master actor, the `httpSequence`
> field was lazily initialized in `ProcessBase::consume()` and afterwards
> a call to `ProcessBase::_consume()` was dispatched, where it was
> assumed that `httpSequence` is already initialized.
> 
> However, during this test the master actor would be destroyed and a
> new actor would be spawned with the same PID. The dispatched method
> would be called on this new actor and find `httpSequence` to be not
> initialized, leading to a crash.
> 
> This patch introduces a call to `Clock::settle()` after the master
> is shut down to ensure the outstanding `_consume()` gets discarded
> before starting the new master actor.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
> 
> 
> Diff: https://reviews.apache.org/r/66799/diff/1/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests --gtest_filter="*MasterAPITest*MasterFailover*" 
> --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67004: Added an option to keep downloaded patches to apply-reviews.py.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On May 8, 2018, 12:54 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67004/
> ---
> 
> (Updated May 8, 2018, 12:54 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By default, the apply-reviews.py script will delete all patch files it 
> downloads. However, when a patch fails to apply, it might be desired to edit 
> and apply it manually. This change will make it easier to do so.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py b6801b926d9fc32172f102586d49e0464da71359 
> 
> 
> Diff: https://reviews.apache.org/r/67004/diff/1/
> 
> 
> Testing
> ---
> 
> Ran `./support/apply-reviews.py` script with and without the new option.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 66984: Added the ability to exhaustively allocate declined resources.

[Meng Zhu]

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

(Updated May 8, 2018, 9:55 a.m.)


Review request for mesos, Benjamin Mahler, Kapil Arya, Till Toenshoff, and 
Vinod Kone.


Changes
---

Slip fix.


Repository: mesos


Description
---

Schedulers that are below their fair share will continue to get
allocated resources even if they don't need those resources. The
expected behavior here is that schedulers will decline those resources
for a long time (or forever). Not all schedulers do this, however,
which means that some schedulers might get _starved_ of
resources. Technically these schedulers are already at or above their
fair share, but some operators feel that this is keeping the cluster
underutilized.

Rather than guarantee that all schedulers will decline with large
timeouts this patch ensures that all other schedulers will see
resources from the declined agent before they see resources from that
agent again, even if there are now more resources available on that
agent.

To this end, each agent maintains an exclusion set of frameworks
that declined its allocation earlier and skips them during the
allocation. Note, a framework here is associated with each of
its roles. If a framework declined resources allocated to one
of its roles, it can still get allocations for its other roles.

Note, an agent's exclusion set will be cleared if its attribute
changes.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
955ae3e6a9e3c790fb260311ec4b4ef725a826d3 
  src/master/allocator/mesos/hierarchical.cpp 
1000968be6a2935a4cac571414d7f06d7df7acf0 


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

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


Testing
---

make check
Dedicated test in #66994


Thanks,

Meng Zhu

Re: Review Request 66934: Added token-based authentication to resource provider tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66932, 66933, 66934]

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

- Mesos Reviewbot


On May 8, 2018, 1:09 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66934/
> ---
> 
> (Updated May 8, 2018, 1:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8774
> https://issues.apache.org/jira/browse/MESOS-8774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If compiled with SSL support, resource provider tests will use
> authentication. The 'MockResourceProvider' has been updated to create a
> JWT when started.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_resource_provider_config_api_tests.cpp 
> 6ced582e14966eea3bb109e2e2de0e554acc2968 
>   src/tests/api_tests.cpp 1ed26a7d796be3e3d0a44d107d2f769a39ff450a 
>   src/tests/executor_http_api_tests.cpp 
> 51cca7a90b3c5c5380ede0f17d66b9fdfeb88c27 
>   src/tests/files_tests.cpp 4286e58d3407a2b82cf720651697ee635b10ad63 
>   src/tests/logging_tests.cpp 886dcd0d6d6ccf509c98578e8b86b1c6ecd5775f 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 937bab08c9bef1a2a6a400979dcf0895412168f5 
>   src/tests/master_tests.cpp 0f6042ca15ac5862d5dc73f113bbe23504eda65c 
>   src/tests/mesos.hpp b945edf07e98422bf6bfceb6d0d9fa2ff188aece 
>   src/tests/metrics_tests.cpp 55065cd11ece50e5b79a8e72a4075ebc93817794 
>   src/tests/operation_reconciliation_tests.cpp 
> 39cf1888ed1ad9395203c6f9c6ffd87fc7623195 
>   src/tests/resource_provider_manager_tests.cpp 
> c2045f2ba24f3e4b959115f23b706e733a75fea8 
>   src/tests/slave_tests.cpp 9e398842e57fda67aaed9fd23e15eef4ce180e05 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ccb114aacc904998dfeef4128f6d38dfb3c865c7 
> 
> 
> Diff: https://reviews.apache.org/r/66934/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 66634: Explicitly marked functions `override` in cgroup subsystems.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On April 16, 2018, 11:03 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66634/
> ---
> 
> (Updated April 16, 2018, 11:03 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds `override` keywords to functions overriding `virtual`
> base class functions. This explicitness helps in a subsequent patch in
> refactoring the `Subsystem` base class interface.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
> ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
> 282a76189579d3ddd61f4aad210ce8e876ff0c25 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> e4977da476a04a708a2e9b9dcc5a2f85da5867f4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuset.hpp 
> b5d712a8eb8ba74092184024e3b40ad9ba1b7584 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
> 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/hugetlb.hpp 
> 27407133e7315dccf1efe2440cc1bf79c51e7dca 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> d79fda3cf852e50e75dbeeeaeae50b1f471a55a4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
> bcaa16142c4e9882dea88e70095cfb7f223401ef 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_prio.hpp 
> 002c689503d45622cba437c851561f5ec7dc8a12 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
> 14c0e79f045fb826d20e476772e017439977dded 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/pids.hpp 
> cb6c91920355d3d5599f8a3cf0ce2ac5eee18d37 
> 
> 
> Diff: https://reviews.apache.org/r/66634/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66821: Added framwork metrics helper normalize() and getPrefix().

[Gaston Kleiman]

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



There's a typo in the RR description: s/framwork/framework/


src/master/metrics.hpp
Lines 226-227 (patched)


This is done in https://reviews.apache.org/r/66882/, can we squash both 
patches?


- Gaston Kleiman


On April 30, 2018, 11:41 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66821/
> ---
> 
> (Updated April 30, 2018, 11:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framwork metrics helper normalize() and getPrefix().
> 
> 
> Diffs
> -
> 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
> 
> 
> Diff: https://reviews.apache.org/r/66821/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 65811: Add `path::normalize` to stout for normalizing path (for POSIX only now)

[Jason Lai]

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

(Updated May 8, 2018, 6:41 p.m.)


Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James 
Peach, and Zhitao Li.


Changes
---

Renamed `path::clean` to `path::normalize` per review comment.


Summary (updated)
-

Add `path::normalize` to stout for normalizing path (for POSIX only now)


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


Repository: mesos


Description (updated)
---

Add `path::normalize` to stout for normalizing path (for POSIX only now).


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
27438d31617b3b78bf3d4deffd25c93340610e8d 


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

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


Testing
---


Thanks,

Jason Lai

Re: Review Request 65812: Added an overloaded version of `os::realpath` to stout

[Jason Lai]

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

(Updated May 8, 2018, 6:43 p.m.)


Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James 
Peach, and Zhitao Li.


Changes
---

Rename `path::clean` to `path::normalize`.


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


Repository: mesos


Description
---

Added an overloaded version of `os::realpath` to stout.

The new `os::realpath` function is used for evaluating real path
within a scoped root directory.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/posix/realpath.hpp 
31352cefc5b8d0ccd9af8f6dabdec4a959fded32 


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

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


Testing
---


Thanks,

Jason Lai