Re: Review Request 60871: Added double-checked locking for filter.

[Benjamin Mahler]

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


Ship it!




- Benjamin Mahler


On July 15, 2017, 12:13 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60871/
> ---
> 
> (Updated July 15, 2017, 12:13 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
> https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When running in"production" a filter will not be (likely) be set yet
> with the current code the threads will experience head-of-line
> blocking becaues they'll all queue up on the mutex. Test code will
> still queue up but we don't care about the performance in these
> situations!
> 
> Note that this patch also moves the filter into ProcessManager in the
> effort towards eliminating globals.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/filter.hpp 
> cafa7be065244fe428dee78c5e02e4513f8c1c35 
>   3rdparty/libprocess/src/process.cpp 
> 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d 
> 
> 
> Diff: https://reviews.apache.org/r/60871/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 60791: Add fetcher cache space usage metrics.

[James Peach]

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

(Updated July 19, 2017, 3:05 a.m.)


Review request for mesos, Joseph Wu and Jiang Yan Xu.


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


Repository: mesos


Description (updated)
---

Add fetcher metrics to track the (constant) size of the cache
size and the (varying) amount of cache space use. The cache size
is published as `containerizer/fetcher/cache_size_total_bytes`
and the used space is `containerizer/fetcher/cache_size_used_bytes`.


Diffs (updated)
-

  docs/monitoring.md 38b8093ef683b5de65cbfa5330a6bbd1c9e10f12 
  src/slave/containerizer/fetcher.cpp 6a664e0657a19d27afac98fd5298d6a18aabe43f 
  src/slave/containerizer/fetcher_process.hpp 
3ed7dc9db5b64b72881249767c0356a3bc5370e7 
  src/tests/fetcher_cache_tests.cpp 1c654e511d2079de746ac97a2c2718e1b926768e 


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

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


Testing
---

make check (Fedora 26).


Thanks,

James Peach

Review Request 60956: Refactored fetcher cache metrics.

[James Peach]

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

Review request for mesos, Joseph Wu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Refactored fetcher cache metrics to be more consistent with how
metrics are handled in other processes. Added an inner "Metrics"
class to hold the metrics, and renamed them to snake_case following
the published metrics names.


Diffs
-

  src/slave/containerizer/fetcher.cpp 6a664e0657a19d27afac98fd5298d6a18aabe43f 
  src/slave/containerizer/fetcher_process.hpp 
3ed7dc9db5b64b72881249767c0356a3bc5370e7 


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


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60830: Replaced std::map with hashmap for ProcessBase::handlers.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 18, 2017, 11:44 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60830/
> ---
> 
> (Updated July 18, 2017, 11:44 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
> https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced std::map with hashmap for ProcessBase::handlers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 944fcc6449edfd022db4048f70a13aff4a1ff345 
> 
> 
> Diff: https://reviews.apache.org/r/60830/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 60898: Added more tests for agent reregistration.

[James Peach]

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




src/tests/slave_tests.cpp
Lines 3327 (patched)


You need a corresponding `Clock::resume()`?



src/tests/slave_tests.cpp
Lines 3415 (patched)


You need a corresponding `Clock::resume()`?



src/tests/slave_tests.cpp
Lines 3431 (patched)


This comment seems wrong since you are not intercepting a registry 
operation.



src/tests/slave_tests.cpp
Lines 3440 (patched)


Is there a way you can set an expectation that a registry operation is not 
made?


- James Peach


On July 16, 2017, 6:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60898/
> ---
> 
> (Updated July 16, 2017, 6:29 p.m.)
> 
> 
> Review request for mesos, James Peach and Neil Conway.
> 
> 
> Bugs: MESOS-7711
> https://issues.apache.org/jira/browse/MESOS-7711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These new tests are specifically for verifying whether the registrar
> is involved when an agent reregisters, depending on whether it has
> been marked unreachable.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 053a14d1ea920058563173de7e0fa644fdbd8d96 
> 
> 
> Diff: https://reviews.apache.org/r/60898/diff/2/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60821: Introduced a "no sender" UPID.

[James Peach]

> On July 17th, 2017, 2:14 p.m. PDT, Jiang Yan Xu wrote:
> 
> Clarified with Joseph offline. The proposal is to just fix this specific 
> situation in MESOS-7753 (I submitted /r/60917/ for review). I'll drop this 
> review for now and we can revisit this when we have another use case.
> While I dropped this for now, here I'd like to summarize the discussion.
> So the discussion is around how to make these process::post methods less 
> error-prone:
> // post(1)
> void post(const UPID& to,
>   const std::string& name,
>   const char* data = nullptr,
>   size_t length = 0);
> 
> // post(2)
> void post(const UPID& from,
>   const UPID& to,
>   const std::string& name,
>   const char* data = nullptr,
>   size_t length = 0);
> 
> 
> Given post(1) may be confused with send, which most Mesos "production" code 
> uses:
>   void send(
>   const UPID& to,
>   const std::string& name,
>   const char* data = nullptr,
>   size_t length = 0);
> 
> 
> We can either a) eliminate post(1) and do
> post(UPID::anonymous(), to, data);
> 
> or b) hide the implementation under post(1) so you would do:
> post(to, data);
> 
> 
> Explicitness is good and at this point I am not preferring b) over a), but 
> just would like to explore whether either is a good enough fix.
> Consider:
> 
>   • Even if we eliminate post(2), is post(2) good enough? Should we put 
> it in a different namespace to prevent misuse (currently most uses of either 
> post are in tests)?

No, I already struggle to find symbols in the forest of namespaces.

>   • What's going to prevent the caller from doing post(UPID(), to, data), 
> is Try post(from, to, data) with validation going to help?

Maybe there's no good use case for an empty UPID? Alternatively `post` could 
`CHECK` the UPID?

>   • Could UPID::anonymous() be misused for some unintented purposes?
>   • If we use a different method name for post(1), something like 
> noSenderPost(to, data) (I am sure we can come up with a better name), is it 
> still confusing?

If `post` for when you send a message without a process. Maybe the right 
approach is to remove post(2) and define post(1) to always use the anonymous 
sender?

>   • Finally, is any of these better than clearly document the APIs with 
> caution notes about empty UPIDs?

Usually I refer to Rusty Russell' API levels. If we can make the API impossible 
to misuse or throw a runtime or compile time error, that's better than simply 
documenting the traps.

J

Re: Review Request 60854: Changed the way tests capture agent state transitioning.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On July 13, 2017, 11 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60854/
> ---
> 
> (Updated July 13, 2017, 11 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-7711
> https://issues.apache.org/jira/browse/MESOS-7711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing way captures the registrar operation which may not
> occur after MESOS-7711 but regardless of that, capturing the agent
> authorization is equivalent and arguably more straightforward.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 
>   src/tests/reconciliation_tests.cpp 9c36b9a0558ab56ce45fdb93189a1b99f487ba4d 
> 
> 
> Diff: https://reviews.apache.org/r/60854/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60821: Introduced a "no sender" UPID.

[Jiang Yan Xu]

> On July 12, 2017, 5:27 p.m., James Peach wrote:
> > I think that a better approach is to define a static `UPID 
> > UPID::anonymous()` function that returns a well-defined anonymous UPID for 
> > the system. Remove all the sending functions that don't specify a `from` 
> > UPID and force them to explicitly pass the anonymous UPID. This makes the 
> > used of anonymous message sends explicit which I think is preferable to 
> > this implicit approach.
> 
> Jiang Yan Xu wrote:
> Here's my thinking:
> 
> - In libprocess the PID is usually not constructed directly, aside from 
> being parsed but that's also hidden from the users of the library. Users of 
> libprocess tend to get the PID from the process that owns that PID or from 
> `spawn`. Afterwards it is used as an opaque ID. Therefore I feel like more 
> consistent with the current architecture to not expose it at all. Having to 
> know to use a speical ID is less straighfoward than calling a method that 
> doesn't take a `from` IMO.
> 
> - Right now the PID is just a simple data struct and not guarded by a 
> `process::initialize()` call. This special UPID and PID uses the value of 
> `__address__`, which initialized in `process::initialize()`. Therefore even 
> if we expose this anonymous / no sender PID directly, I feel it's probably 
> going to be constructed by something other than a static UPID method. This is 
> a minor point though.
> 
> James Peach wrote:
> * Anything that calls `post` is going to have to initialize `libprocess`, 
> so initializing `libprocess` when we generate the anonymous UPID seems OK 
> too. `UPID process::anonymous()`.
> * Sending anonymous messages is a special case, so it should be explicit. 
> `post` is also a special case since you already have to specify a target 
> UPID. I argue that explicit is better than implicit and 1 way to `post` is 
> better than 2.
> 
> Jiang Yan Xu wrote:
> Yeah I think the main thing I am arguing for here is that as you said, 
> the purpose of this is to support "Sending anonymous messages / message with 
> no sender address". IMO the sentence from the user's POV translates to a 
> method (and we already have this method), not an UPID.
> 
> In othe words, this special 
> [process::post](https://github.com/apache/mesos/blob/5645fddf07eab0fcd195a3514e9e5ad3ef25628d/3rdparty/libprocess/include/process/process.hpp#L626-L637)
>  overload serves a special purpose that
> - We currently have a use case directly
> - I don't see how it could be incorrectly used for other unintended ways.
> 
> 
> In contrast, exposing a special UPID to user:
> - I don't see how the UPID can be used for another legit purpose.
> - I can see it could used incorrectly by the user.
> 
> 
> I do support explictness, but I think saying a method "sends message with 
> no sender address" is pretty explicit too and without exposing too much 
> flexibility and a concept that could be hard(er) to undertand (than this).
> 
> Having said that, I am not adamant on the current design. Given another 
> vote/idea I can switch.
> 
> Joseph Wu wrote:
> If the eventual goal is to enable 
> `LIBPROCESS_REQUIRE_PEER_ADDRESS_IP_MATCH` by default, it would make sense to 
> either (A) add the special anonymous sender, or (B) remove the sender-less 
> `process::post` variant entirely.
> 
> In terms of implementing (A), I would opt for an implementation that is 
> explicit.  This is primarily due to the ambiguity from calling 
> `process::post(to, message)` within the context of a libprocess Process, vs a 
> main thread or test body.
> 
> That being said, we don't have any protections or validation regarding 
> the `from` field anyway.  Any process can say `process::post(not_me /* :P */, 
> to, message)` and that would be considered valid.
> 
> ---
> 
> I agree with the explicit anonymous sender approach.  But I don't think 
> we necessarily need to implement the anonymous sender yet.
> 
> I think going with the explicit route will give us an excuse to 
> double-check any places with no sender address/UPID.  If we see no reason to 
> keep it, then we could add the explicit anonymous sender if we ever find a 
> reason to include it.
> 
> Jiang Yan Xu wrote:
> Not sure if I 100% understand the recommendation here. I think that it is:
> 
> - We implement `UPID process::anonymous()` and use it to avoid 
> `LIBPROCESS_REQUIRE_PEER_ADDRESS_IP_MATCH` check failure.
> 
> > But I don't think we necessarily need to implement the anonymous sender 
> yet.
> 
> Did you mean to say that we do "(B) remove the sender-less process::post 
> variant entirely" later after we "double-check any places with no sender 
> address/UPID"?
> 
> Jiang Yan Xu wrote:
> Clarified with Joseph offline. The proposal is to just fix this specific 
> situation in MESOS-7753 (I submitted /r/60917/ for review). I'll drop 

Re: Review Request 60791: Add fetcher cache space usage metrics.

[James Peach]

> On July 18, 2017, 9:16 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/fetcher.cpp
> > Lines 266-267 (original), 266-275 (patched)
> > 
> >
> > Outside the fetcher I think we use a convention like this: 
> > https://github.com/apache/mesos/blob/d5b85d2b9063b5fb4a40108ec4801cc5ed2f5155/src/master/registrar.cpp#L146-L195
> > 
> > i.e., 
> > 
> > - Group metrics in a `Metrics` nested class. (Later when you have more 
> > of them and would like to pull them out into a different file you can 
> > easily do so).
> > - Snake casing which is consistent with the metric name.
> > - For gauges, defer to an internal private handler prefixed by a `_`.
> > 
> > Any reason to diverge?
> > 
> > (Aside from `cache_size_total` which is const)

This specifically doesn't `defer` due to the performance overhead of that, 
compared to just making a member function call.


> On July 18, 2017, 9:16 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/fetcher.cpp
> > Lines 272-275 (patched)
> > 
> >
> > Aside from styling/convention, would this require defer?

No, this is avoiding the defer on purpose.


> On July 18, 2017, 9:16 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/fetcher.cpp
> > Lines 293-295 (patched)
> > 
> >
> > Why await?

This synchronizes the metric removal so that it can't be evaluated after the 
process has been destroyed.


> On July 18, 2017, 9:16 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/fetcher_process.hpp
> > Lines 147-148 (patched)
> > 
> >
> > If these are used only for monitoring, should we just follow the 
> > convention mentioned above and make them private?

We already had `availableSpace()`, so this extends that to a consistent API 
family.


- James


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


On July 18, 2017, 2:13 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60791/
> ---
> 
> (Updated July 18, 2017, 2:13 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7782
> https://issues.apache.org/jira/browse/MESOS-7782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add fetcher metrics to track the (constant) size of the cache size
> and the (varying) amount of cache space use. The cache size is
> published as `containerizer/fetcher/cache_size_total` and the used
> space is `containerizer/fetcher/cache_size_used`. Both of these
> metrics are in MB to be consistent with other disk space metrics.
> We manually convert to fractional MB so that small amounts of space
> usage don't get truncated away.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 38b8093ef683b5de65cbfa5330a6bbd1c9e10f12 
>   src/slave/containerizer/fetcher.cpp 
> 6a664e0657a19d27afac98fd5298d6a18aabe43f 
>   src/slave/containerizer/fetcher_process.hpp 
> 3ed7dc9db5b64b72881249767c0356a3bc5370e7 
>   src/tests/fetcher_cache_tests.cpp 1c654e511d2079de746ac97a2c2718e1b926768e 
> 
> 
> Diff: https://reviews.apache.org/r/60791/diff/3/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60913: Adds support for OpenSSL's ECDH handshake.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60913]

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

- Mesos Reviewbot


On July 18, 2017, 12:13 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60913/
> ---
> 
> (Updated July 18, 2017, 12:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support for Elliptic Curve Diffie Hellman algorithm requires extra
> configuration parameters which weren't part of Mesos.
> 
> This patch enables the extra configuration to Mesos in order to
> support ECDH algorithm, it also adds the ssl flag
> `LIBPROCESS_SSL_ECDH_CURVES` which allows for the specification of
> a specific elliptic curve.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
>   3rdparty/libprocess/src/openssl.cpp 
> e6f17e4591f573186e1dc9697e1e7b60a841fe4f 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 8a14dcb865dfab34fb4d0d51f42a28a913fb7ace 
> 
> 
> Diff: https://reviews.apache.org/r/60913/diff/3/
> 
> 
> Testing
> ---
> 
> ```shell
> make check
> ```
> 
> Launched Mesos with only ECDHE handshake ciphers enabled
> 
> ```shell
> LIBPROCESS_SSL_ENABLED=1 \
> LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
> LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
> LIBPROCESS_SSL_CIPHERS="ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA"
>  \
> ./bin/mesos-master.sh \
> --work_dir=/tmp/mesos/master \
> --log_dir=/tmp/mesos/master/log
> ```
> 
> Then in another shell:
> 
> ```shell
> http -v --verify=no https://${MESOS_MASTER_IP}:5050/state
> 
> # Launches a browser.
> open https://${MESOS_MASTER_IP}:5050/state
> 
> # List the set of supported ciphers.
> # Expected output:
> # >  Starting Nmap 7.50 ( https://nmap.org ) at 2017-07-18 11:41 CEST
> # >  Nmap scan report for ${MESOS_MASTER_HOSTNAME} (${MESOS_MASTER_IP})
> # >  Host is up (0.13s latency).
> # >  rDNS record for ${MESOS_MASTER_IP}: ${MESOS_MASTER_HOSTNAME}
> # >  
> # >  PORT STATE SERVICE
> # >  5050/tcp open  mmcc
> # >  | ssl-enum-ciphers:
> # >  |   TLSv1.2:
> # >  | ciphers:
> # >  |   TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
> # >  | compressors:
> # >  |   NULL
> # >  | cipher preference: server
> # >  |_  least strength: A
> # >  
> # >  Nmap done: 1 IP address (1 host up) scanned in 1.87 seconds
> wget https://svn.nmap.org/nmap/scripts/ssl-enum-ciphers.nse
> nmap --script ssl-enum-ciphers.nse -p 5050 ${MESOS_MASTER_IP}
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 60953: Updated libprocess for the new 3rdparty moodycamel/concurrentqueue.

[Benjamin Mahler]

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

Review request for mesos, Andrew Schwartzmeyer and Benjamin Hindman.


Repository: mesos


Description
---

This is a header only library that provides a lock-free concurrent
queue which we can leverage in libprocess to improve performance.

The update to the CMake build will be added separately from this
change, note that this does not break the CMake build, since it
is not used yet. The plan is to have inclusion of concurrentqueue.h
be guarded by a configure flag to begin with, which means that it
will not break the CMake build, it will however prevent CMake
users from choosing to use the concurrent queue until the CMake
integration is complete.


Diffs
-

  3rdparty/libprocess/3rdparty/Makefile.am 
8a2adf2e95ce0d63a32f562bf6af1685abc17ab5 
  3rdparty/libprocess/Makefile.am d2b2ed21b912247a8d926e67f49e2cdef7ea11f6 
  3rdparty/libprocess/configure.ac cec01908db73418a549cd79d1d701a19dd623821 


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


Testing
---

make tests


Thanks,

Benjamin Mahler

Re: Review Request 60831: Removed extra/unnecessary allocations of Message.

[Benjamin Hindman]

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

(Updated July 18, 2017, 11:44 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Removed extra/unnecessary allocations of Message.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
8afe6266eb0dc5a17af35d79efb6bfdf9e6a0ee9 
  3rdparty/libprocess/include/process/gmock.hpp 
e9af943b39436f365fe687301febb5c7fbefffc4 
  3rdparty/libprocess/include/process/protobuf.hpp 
ba6e6d6504250a2609b336f3e9854cfbe5da52ec 
  3rdparty/libprocess/src/encoder.hpp ea629d72a68e093343562db1ef7e5d00c723f03b 
  3rdparty/libprocess/src/process.cpp 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d 
  3rdparty/libprocess/src/tests/process_tests.cpp 
38d787a083a5eb31e922d283f4b4bed2bd62eb0a 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
77df385d3388788658fa40d033816e1fbb8d8f2c 


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

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 60830: Replaced std::map with hashmap for ProcessBase::handlers.

[Benjamin Hindman]

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

(Updated July 18, 2017, 11:44 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Replaced std::map with hashmap for ProcessBase::handlers.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
944fcc6449edfd022db4048f70a13aff4a1ff345 


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

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 60831: Removed extra/unnecessary allocations of Message.

[Benjamin Hindman]

> On July 15, 2017, 1:25 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Line 455 (original), 455 (patched)
> > 
> >
> > Why did you decide to take `Message` here instead of `Message&&`? It 
> > seems a little odd that both `send` and `send_connect` took a pointer but 
> > now they've diverged in signature?

I cleaned that up just now. We have to do a single copy no matter what, but I 
think the way I updated the code reads the easiest now.


- Benjamin


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


On July 15, 2017, 12:13 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60831/
> ---
> 
> (Updated July 15, 2017, 12:13 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
> https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed extra/unnecessary allocations of Message.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 8afe6266eb0dc5a17af35d79efb6bfdf9e6a0ee9 
>   3rdparty/libprocess/include/process/gmock.hpp 
> e9af943b39436f365fe687301febb5c7fbefffc4 
>   3rdparty/libprocess/include/process/protobuf.hpp 
> ba6e6d6504250a2609b336f3e9854cfbe5da52ec 
>   3rdparty/libprocess/src/encoder.hpp 
> ea629d72a68e093343562db1ef7e5d00c723f03b 
>   3rdparty/libprocess/src/process.cpp 
> 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 38d787a083a5eb31e922d283f4b4bed2bd62eb0a 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 77df385d3388788658fa40d033816e1fbb8d8f2c 
> 
> 
> Diff: https://reviews.apache.org/r/60831/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53479: Perform agent GC asynchronously.

[Jacob Janco]

> On July 18, 2017, 10:50 p.m., Jiang Yan Xu wrote:
> > Committing with these additional edits.

Awesome, thanks!


- Jacob


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


On July 18, 2017, 10:15 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated July 18, 2017, 10:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/10/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 53479: Perform agent GC asynchronously.

[Jiang Yan Xu]

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


Ship it!




Committing with these additional edits.


src/slave/gc.cpp
Lines 20 (patched)


We don't need this anymore.



src/slave/gc.cpp
Lines 25 (patched)


This should be below foreach.hpp.



src/slave/gc.cpp
Lines 202 (patched)


Included `#include ` for this.



src/slave/gc.cpp
Lines 204 (patched)


Removed this logging as it doesn't provide much additional info on top of 
existing logging, other than the fact that some callback has been called.


- Jiang Yan Xu


On July 18, 2017, 3:15 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated July 18, 2017, 3:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/10/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 60825: Performance optimizations for message passing.

[Benjamin Hindman]

> On July 15, 2017, 2:08 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Line 560 (original), 561-562 (patched)
> > 
> >
> > This seems to suggest there will be a comment about it in the 
> > run_queue.hpp header but there isn't?

Added a comment, thanks!


> On July 15, 2017, 2:08 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/run_queue.hpp
> > Lines 73 (patched)
> > 
> >
> > const?

Agreed.


- Benjamin


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


On July 18, 2017, 10:22 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60825/
> ---
> 
> (Updated July 18, 2017, 10:22 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
> https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Optimizations include:
> 
> * Factored out run queue and introduced lock free implementation. This
>   also required adding the concept of an `epoch` to support proper
>   settling and refactoring the increments/decrements of `running` to
>   make it easier to reason about.
> 
> * Replaced the use of a condition variable (the `Gate`) with a
>   semaphore.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c2190b40b748c19a7db1605f69449cc59f95d1cb 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 2c46d43ed8418f128f12fad10151700611e81b1e 
>   3rdparty/libprocess/configure.ac cec01908db73418a549cd79d1d701a19dd623821 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 670dc1510c0ca656abd7f7dfc781f799e54303ec 
>   3rdparty/libprocess/src/concurrentqueue.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d 
>   3rdparty/libprocess/src/run_queue.hpp PRE-CREATION 
>   3rdparty/libprocess/src/semaphore.hpp PRE-CREATION 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
> 
> 
> Diff: https://reviews.apache.org/r/60825/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 60825: Performance optimizations for message passing.

[Benjamin Hindman]

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

(Updated July 18, 2017, 10:22 p.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Performance optimizations for message passing.


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


Repository: mesos


Description (updated)
---

Optimizations include:

* Factored out run queue and introduced lock free implementation. This
  also required adding the concept of an `epoch` to support proper
  settling and refactoring the increments/decrements of `running` to
  make it easier to reason about.

* Replaced the use of a condition variable (the `Gate`) with a
  semaphore.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c2190b40b748c19a7db1605f69449cc59f95d1cb 
  3rdparty/libprocess/cmake/ProcessConfigure.cmake 
2c46d43ed8418f128f12fad10151700611e81b1e 
  3rdparty/libprocess/configure.ac cec01908db73418a549cd79d1d701a19dd623821 
  3rdparty/libprocess/src/CMakeLists.txt 
670dc1510c0ca656abd7f7dfc781f799e54303ec 
  3rdparty/libprocess/src/concurrentqueue.hpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d 
  3rdparty/libprocess/src/run_queue.hpp PRE-CREATION 
  3rdparty/libprocess/src/semaphore.hpp PRE-CREATION 
  configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 


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

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 53479: Perform agent GC asynchronously.

[Jacob Janco]

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

(Updated July 18, 2017, 10:15 p.m.)


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


Changes
---

Final.


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


Repository: mesos


Description
---

- Previously, rmdir operations were serialized
  through the garbage collector. Expensive removal
  events had the potential to delay task launch.
- MESOS-6549


Diffs (updated)
-

  src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
  src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 


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

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


Testing
---

`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
--gtest_break_on_failure`


Thanks,

Jacob Janco

Re: Review Request 53479: Perform agent GC asynchronously.

[Jacob Janco]

> On July 17, 2017, 8:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > 
> >
> > I see that `promise == that.promise` here is required by the Multimap 
> > but does it make sense to have `PathInfo` equality depend only on the path?
> > 
> > I don't think `Promise` is supposed to be comparable.
> 
> Jiang Yan Xu wrote:
> OK I think strictly speaking we shouldn't assume that there are no equal 
> values under the same key in a Multimap (or std::multimap).
> 
> This case shouldn't happen with our GC so we should probably insert some 
> hard `CHECK`s for it (in a separate patch) but I don't think removing 
> `promise == that.promise` has anything to do with it, i.e., the problem 
> exists today.
> 
> Jacob Janco wrote:
> Updating and removing the blocking dependency. Actually, I don't think we 
> even use this operator...why not just remove this and add the checks like you 
> said.
> 
> Jiang Yan Xu wrote:
> I think it's required by Multimap.

Per discussion: Now that we store a pointer remove this equality.


- Jacob


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


On July 18, 2017, 12:23 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated July 18, 2017, 12:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/9/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 60936: Removed stale FetcherProcess::setSpace declaration.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On July 17, 2017, 7:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60936/
> ---
> 
> (Updated July 17, 2017, 7:15 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7782
> https://issues.apache.org/jira/browse/MESOS-7782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed the stale declaration of FetcherProcess::setSpace(). The
> implementation was previously removed in 6db493c07.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher_process.hpp 
> 3ed7dc9db5b64b72881249767c0356a3bc5370e7 
> 
> 
> Diff: https://reviews.apache.org/r/60936/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On July 18, 2017, 9:38 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60822/
> ---
> 
> (Updated July 18, 2017, 9:38 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter support for the '/slaves', '/frameworks' and
> '/slave/containers' endpoints.
> 
> This allows slaves, frameworks and containers to be queried
> by ID.
> 
> If no ID is specified, all records are returned, consistent
> with current behavior.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
>   src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
>   src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 
> 
> 
> Diff: https://reviews.apache.org/r/60822/diff/9/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60847: Added test cases for /slaves, /slave/containers, /frameworks endpoints.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On July 18, 2017, 9:38 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60847/
> ---
> 
> (Updated July 18, 2017, 9:38 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter test cases for '/slaves' and '/frameworks' on
> the master, and '/slave/containers' endpoint on the slave.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 
>   src/tests/slave_tests.cpp 035db18db3a64a9e358c1c54cc18a4bdeb85d8bf 
> 
> 
> Diff: https://reviews.apache.org/r/60847/diff/4/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> Passed 'GTEST_FILTER="MasterTest.FrameworksEndpointQueryFramework" make check 
> -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterTest.FrameworksEndpointQueryFramework" 
> --gtest_repeat=1000 --gtest_break_on_failure'
> Passed 'GTEST_FILTER="MasterTest.SlavesEndpointQuerySlave" make check -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterTest.SlavesEndpointQuerySlave" --gtest_repeat=1000 
> --gtest_break_on_failure'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

[Greg Mann]

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




src/master/http.cpp
Lines 444 (patched)


Spaces needed.



src/master/http.cpp
Lines 456 (patched)


Spaces needed.


- Greg Mann


On July 18, 2017, 9:38 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60822/
> ---
> 
> (Updated July 18, 2017, 9:38 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter support for the '/slaves', '/frameworks' and
> '/slave/containers' endpoints.
> 
> This allows slaves, frameworks and containers to be queried
> by ID.
> 
> If no ID is specified, all records are returned, consistent
> with current behavior.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
>   src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
>   src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 
> 
> 
> Diff: https://reviews.apache.org/r/60822/diff/8/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

[Greg Mann]

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




src/slave/http.cpp
Line 2131 (original), 2127 (patched)


Indent this line one fewer space.


- Greg Mann


On July 18, 2017, 6:24 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60822/
> ---
> 
> (Updated July 18, 2017, 6:24 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter support for the '/slaves', '/frameworks' and
> '/slave/containers' endpoints.
> 
> This allows slaves, frameworks and containers to be queried
> by ID.
> 
> If no ID is specified, all records are returned, consistent
> with current behavior.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
>   src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
>   src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 
> 
> 
> Diff: https://reviews.apache.org/r/60822/diff/6/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60791: Add fetcher cache space usage metrics.

[Jiang Yan Xu]

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



Haven't looked at tests but perhaps we can discuss the current issues first.


docs/monitoring.md
Lines 1258-1270 (patched)


We do have a lot of disk related metrics in "bytes".

Inconsisteny does exist but the line is strictly between the "abstract" 
resources and "physical" resources. 

All "abstract" disk/memory resources currently don't have units in the name 
and are in MBs. This is because the flags and resource quantity calculation in 
Mesos are in MBs. Not having the unit in the name is bad IMO but many of these 
are historical.

The "physical" resource metrics all have units and are in bytes:

```
disk_limit_bytes
disk_used_bytes
system/mem_free_bytes
system/mem_free_bytes
```

So despiste the inconsistency (which is not going to be fixed by using MB 
here), I think we should use bytes here and have the unit explicit in the 
metric name because it's clearer.

If we make things consistent, I would vote to have units with all resource 
metrics.



src/slave/containerizer/fetcher.cpp
Lines 266-267 (original), 266-275 (patched)


Outside the fetcher I think we use a convention like this: 
https://github.com/apache/mesos/blob/d5b85d2b9063b5fb4a40108ec4801cc5ed2f5155/src/master/registrar.cpp#L146-L195

i.e., 

- Group metrics in a `Metrics` nested class. (Later when you have more of 
them and would like to pull them out into a different file you can easily do 
so).
- Snake casing which is consistent with the metric name.
- For gauges, defer to an internal private handler prefixed by a `_`.

Any reason to diverge?

(Aside from `cache_size_total` which is const)



src/slave/containerizer/fetcher.cpp
Lines 272-275 (patched)


Aside from styling/convention, would this require defer?



src/slave/containerizer/fetcher.cpp
Lines 293-295 (patched)


Why await?



src/slave/containerizer/fetcher_process.hpp
Lines 147-148 (patched)


If these are used only for monitoring, should we just follow the convention 
mentioned above and make them private?


- Jiang Yan Xu


On July 17, 2017, 7:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60791/
> ---
> 
> (Updated July 17, 2017, 7:13 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7782
> https://issues.apache.org/jira/browse/MESOS-7782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add fetcher metrics to track the (constant) size of the cache size
> and the (varying) amount of cache space use. The cache size is
> published as `containerizer/fetcher/cache_size_total` and the used
> space is `containerizer/fetcher/cache_size_used`. Both of these
> metrics are in MB to be consistent with other disk space metrics.
> We manually convert to fractional MB so that small amounts of space
> usage don't get truncated away.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 38b8093ef683b5de65cbfa5330a6bbd1c9e10f12 
>   src/slave/containerizer/fetcher.cpp 
> 6a664e0657a19d27afac98fd5298d6a18aabe43f 
>   src/slave/containerizer/fetcher_process.hpp 
> 3ed7dc9db5b64b72881249767c0356a3bc5370e7 
>   src/tests/fetcher_cache_tests.cpp 1c654e511d2079de746ac97a2c2718e1b926768e 
> 
> 
> Diff: https://reviews.apache.org/r/60791/diff/3/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60847: Added test cases for /slaves, /slave/containers, /frameworks endpoints.

[Quinn Leng]

> On July 18, 2017, 8:23 p.m., Greg Mann wrote:
> > src/tests/master_tests.cpp
> > Lines 2523-2524 (patched)
> > 
> >
> > Newline here.

It makes the code look a little strange, one single line of comment surrounded 
by two empty lines.


> On July 18, 2017, 8:23 p.m., Greg Mann wrote:
> > src/tests/master_tests.cpp
> > Line 5866 (original), 5980-5981 (patched)
> > 
> >
> > Newline after this comment.

It makes the code look a little strange, one single line of comment surrounded 
by two empty lines.


- Quinn


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


On July 18, 2017, 6:28 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60847/
> ---
> 
> (Updated July 18, 2017, 6:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter test cases for '/slaves' and '/frameworks' on
> the master, and '/slave/containers' endpoint on the slave.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 
>   src/tests/slave_tests.cpp 035db18db3a64a9e358c1c54cc18a4bdeb85d8bf 
> 
> 
> Diff: https://reviews.apache.org/r/60847/diff/4/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> Passed 'GTEST_FILTER="MasterTest.FrameworksEndpointQueryFramework" make check 
> -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterTest.FrameworksEndpointQueryFramework" 
> --gtest_repeat=1000 --gtest_break_on_failure'
> Passed 'GTEST_FILTER="MasterTest.SlavesEndpointQuerySlave" make check -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterTest.SlavesEndpointQuerySlave" --gtest_repeat=1000 
> --gtest_break_on_failure'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Review Request 60948: Updated test cases for the /tasks endpoint.

[Quinn Leng]

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

Review request for mesos, Alexander Rojas and Greg Mann.


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


Repository: mesos


Description
---

Optimized the code structure for '/tasks' endpoint's test cases.


Diffs
-

  src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 


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


Testing
---

make check


Thanks,

Quinn Leng

Re: Review Request 60820: Added class definition for the IDAcceptor.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On July 18, 2017, 6:21 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60820/
> ---
> 
> (Updated July 18, 2017, 6:21 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit contains the class definitions for IDAcceptor, which is
> used to filter IDs in the '/master/frameworks', '/master/slaves' and
> '/slave/containers' endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
>   src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
> 
> 
> Diff: https://reviews.apache.org/r/60820/diff/9/
> 
> 
> Testing
> ---
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60847: Add test cases for /slaves, /slave/containers, /frameworks endpoints.

[Greg Mann]

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




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


Can remove this comment.



src/tests/master_tests.cpp
Lines 2523-2524 (patched)


Newline here.



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


Can remove this comment.



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


s/recovered_slaves/recovered/



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


Let's use single quotes for consistency:

'recovered_slaves'



src/tests/master_tests.cpp
Lines 3742-3784 (original), 3863-3896 (patched)


Could you break this change out into a separate patch?



src/tests/master_tests.cpp
Line 5866 (original), 5980-5981 (patched)


Newline after this comment.



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


Let's use `driver` instead of `schedDriver` for these variables. This is 
more consistent with the rest of this file.



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


The word "query" here is ambiguous. How about:

"Request with no query parameter."



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


s/shutdown call/teardown call/



src/tests/slave_tests.cpp
Lines 2429 (patched)


How about something more descriptive for this variable name, maybe 
`launchedTask1`?



src/tests/slave_tests.cpp
Lines 2453-2475 (original), 2610-2657 (patched)


If we remove the 'executor_id' field from this JSON, can we have just a 
single `expected` value, and simply do 
`EXPECT_TRUE(value->contains(expected.get()));`?


- Greg Mann


On July 18, 2017, 6:28 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60847/
> ---
> 
> (Updated July 18, 2017, 6:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter test cases for '/slaves' and '/frameworks' on
> the master, and '/slave/containers' endpoint on the slave.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 
>   src/tests/slave_tests.cpp 035db18db3a64a9e358c1c54cc18a4bdeb85d8bf 
> 
> 
> Diff: https://reviews.apache.org/r/60847/diff/3/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> Passed 'GTEST_FILTER="MasterTest.FrameworksEndpointQueryFramework" make check 
> -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterTest.FrameworksEndpointQueryFramework" 
> --gtest_repeat=1000 --gtest_break_on_failure'
> Passed 'GTEST_FILTER="MasterTest.SlavesEndpointQuerySlave" make check -j48'
> Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="MasterTest.SlavesEndpointQuerySlave" --gtest_repeat=1000 
> --gtest_break_on_failure'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

[Greg Mann]

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



Could you update the Summary so that the endpoint paths are consistent? i.e., 
'/containers' instead of '/slave/containers'


src/slave/http.cpp
Lines 2043-2044 (original), 2043-2044 (patched)


Let's break before the capture list:

```
  return authorizeContainer.then(defer(slave->self(),
  [this](const Owned& authorizeContainer) {
```



src/slave/http.cpp
Line 2090 (original), 2088 (patched)


Indented one space too far.



src/slave/http.cpp
Line 2130 (original), 2126 (patched)


Indent this line one more space.


- Greg Mann


On July 18, 2017, 6:24 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60822/
> ---
> 
> (Updated July 18, 2017, 6:24 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter support for the '/master/slaves',
> '/master/frameworks' and '/slave/containers' endpoints.
> 
> This allows slaves, frameworks and containers to be queried
> by ID.
> 
> If no ID is specified, all records are returned, consistent
> with current behavior.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
>   src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
>   src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 
> 
> 
> Diff: https://reviews.apache.org/r/60822/diff/5/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60820: Added class definition for the IDAcceptor.

[Greg Mann]

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




src/common/http.hpp
Lines 197-200 (original), 197-201 (patched)


How about:

"Used to filter results for API handlers. Provides the `accept()` method to 
test whether a supplied ID is equal to a stored target ID. If no target ID is 
provided when the acceptor is constructed, it will accept all inputs."



src/common/http.hpp
Line 219 (original), 208 (patched)


Need spaces:

```
if (id.isSome()) {
```



src/common/http.hpp
Lines 217 (patched)


Spaces here as well.



src/common/http.hpp
Lines 219-220 (patched)


Newline here.


- Greg Mann


On July 18, 2017, 6:21 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60820/
> ---
> 
> (Updated July 18, 2017, 6:21 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit contains the class definitions for IDAcceptor, which is
> used to filter IDs in the '/master/frameworks', '/master/slaves',
> '/master/tasks', and '/slave/containers' endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
>   src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
> 
> 
> Diff: https://reviews.apache.org/r/60820/diff/8/
> 
> 
> Testing
> ---
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 56611: Relax perf version check for Arch Linux.

[Benjamin Mahler]

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




src/tests/containerizer/perf_tests.cpp
Line 133 (original), 137-138 (patched)


Had a question about this comment when reviewing 
[r/60397](https://reviews.apache.org/r/60397/), this seems to imply the stub 
returns a non-zero exit status when run with `--version`? Is that true? It 
would be great to make that explicit in the comment:

```
  // Note that on some systems, perf is a stub that asks you to
  // install the right packages and returns a non-zero exit status.
```


- Benjamin Mahler


On Feb. 28, 2017, 5:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> ---
> 
> (Updated Feb. 28, 2017, 5:43 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Relax perf version check for Arch Linux.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 9c4330b6086abb18f03660fe89a6fb01d8ed 
>   src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb 
>   src/tests/containerizer/perf_tests.cpp 
> d536ecc5cb24787bc3487efb146573cd4f3ded43 
> 
> 
> Diff: https://reviews.apache.org/r/56611/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60847: Add test cases for /slaves, /slave/containers, /frameworks endpoints.

[Quinn Leng]

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

(Updated July 18, 2017, 6:28 p.m.)


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


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


Repository: mesos


Description
---

Added query parameter test cases for '/slaves' and '/frameworks' on
the master, and '/slave/containers' endpoint on the slave.


Diffs (updated)
-

  src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 
  src/tests/slave_tests.cpp 035db18db3a64a9e358c1c54cc18a4bdeb85d8bf 


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

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


Testing
---

Passed 'make check -j48'
Passed 'GTEST_FILTER="MasterTest.FrameworksEndpointQueryFramework" make check 
-j48'
Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
--gtest_filter="MasterTest.FrameworksEndpointQueryFramework" 
--gtest_repeat=1000 --gtest_break_on_failure'
Passed 'GTEST_FILTER="MasterTest.SlavesEndpointQuerySlave" make check -j48'
Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
--gtest_filter="MasterTest.SlavesEndpointQuerySlave" --gtest_repeat=1000 
--gtest_break_on_failure'


Thanks,

Quinn Leng

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

[Quinn Leng]

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

(Updated July 18, 2017, 6:24 p.m.)


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


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


Repository: mesos


Description
---

Added query parameter support for the '/master/slaves',
'/master/frameworks' and '/slave/containers' endpoints.

This allows slaves, frameworks and containers to be queried
by ID.

If no ID is specified, all records are returned, consistent
with current behavior.


Diffs (updated)
-

  src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
  src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
  src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 


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

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


Testing
---

Passed 'make check -j48'


Thanks,

Quinn Leng

Re: Review Request 60820: Added class definition for the IDAcceptor.

[Quinn Leng]

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

(Updated July 18, 2017, 6:21 p.m.)


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


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


Repository: mesos


Description (updated)
---

This commit contains the class definitions for IDAcceptor, which is
used to filter IDs in the '/master/frameworks', '/master/slaves',
'/master/tasks', and '/slave/containers' endpoints.


Diffs
-

  src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
  src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
  src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 


Diff: https://reviews.apache.org/r/60820/diff/8/


Testing
---

make check -j48


Thanks,

Quinn Leng

Re: Review Request 60820: Added class definition for the IDAcceptor.

[Quinn Leng]

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

(Updated July 18, 2017, 6:19 p.m.)


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


Summary (updated)
-

Added class definition for the IDAcceptor.


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


Repository: mesos


Description (updated)
---

This commit contains the class definitions for IDAcceptor, which is
used to filter IDs in the '/master/frameworks', '/master/slaves' and
'/slave/containers' endpoints.


Diffs (updated)
-

  src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
  src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
  src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 


Diff: https://reviews.apache.org/r/60820/diff/8/

Changes: https://reviews.apache.org/r/60820/diff/7-8/


Testing
---

make check -j48


Thanks,

Quinn Leng

Re: Review Request 60820: Added class definition for SlaveID, ContainerID acceptors.

[Quinn Leng]

> On July 18, 2017, 5:54 p.m., Alexander Rojas wrote:
> > src/common/http.hpp
> > Lines 205 (patched)
> > 
> >
> > Why are these default constructors necesary?

It used to be necessary because we were trying to get rid of 'Owned', and using 
the default constructor in the /slave/containers endpoint. Now it has been 
replaced by templated IDAcceptor.


- Quinn


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


On July 14, 2017, 12:10 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60820/
> ---
> 
> (Updated July 14, 2017, 12:10 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit contains the class definitions for SlaveIDAcceptor and
> ContainerIDAcceptor, which are used to filter the
> '/master/frameworks', '/master/slaves' and '/slave/containers'
> endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
>   src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
> 
> 
> Diff: https://reviews.apache.org/r/60820/diff/7/
> 
> 
> Testing
> ---
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60771: Implemented the 'SUBSCRIBE' call in the resource provider manager.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60768, 60769, 60770, 60771]

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

- Mesos Reviewbot


On July 11, 2017, 2:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60771/
> ---
> 
> (Updated July 11, 2017, 2:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7780
> https://issues.apache.org/jira/browse/MESOS-7780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using HTTP, resource providers are expected to subscribe to the resource
> provider manager. The resource provider manager will assign a unique ID
> to subscribed resource providers and use events to communicate with
> them.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp a9bf258a0896bd519b99a689a0bfab64d665172f 
>   src/tests/resource_provider_http_api_tests.cpp 
> 46a3f37ba046201e9d79e8ba037da2cb37f5c31f 
> 
> 
> Diff: https://reviews.apache.org/r/60771/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 60820: Added class definition for SlaveID, ContainerID acceptors.

[Alexander Rojas]

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



Could you reword the commit message with a little bit more of substance?

Like how and why are these acceptors used.


src/common/http.hpp
Lines 205 (patched)


Why are these default constructors necesary?


- Alexander Rojas


On July 14, 2017, 2:10 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60820/
> ---
> 
> (Updated July 14, 2017, 2:10 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit contains the class definitions for SlaveIDAcceptor and
> ContainerIDAcceptor, which are used to filter the
> '/master/frameworks', '/master/slaves' and '/slave/containers'
> endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
>   src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
> 
> 
> Diff: https://reviews.apache.org/r/60820/diff/7/
> 
> 
> Testing
> ---
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60846: Retry logic for unsuccessful `docker rm` during agent recovery.

[Chun-Hung Hsiao]

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

(Updated July 18, 2017, 5:49 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch skips all failures returned from `Docker::rm` during agent
recovery, then schedule retries with an exponential backoff, and go
ahead to unmount the persistent volumes for all Docker containers.

The unit test `DockerContainerizerTest.ROOT_DOCKER_RecoverWithRmFails`
mocks `Docker::rm` to return 4 failures before actually perform
`docker rm` to test if the exponential backoff works correctly.


Diffs (updated)
-

  src/slave/constants.hpp 435c37e28987119c4ef43c9dad61ac052020acd8 
  src/slave/containerizer/docker.cpp 2fe92272d7ac6d916371c55affe24598255f10eb 
  src/slave/flags.hpp bf9adf03847a113306c1df2d71e839b90ada7dd3 
  src/slave/flags.cpp a4c1a0c62af5506bbeb1d26d2fd9ca0ba11d8ad7 
  src/tests/containerizer/docker_containerizer_tests.cpp 
1e85a79f812399270575ea4a64db10e72f40e648 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 60716: Cleaned up authentication acceptor to have one class.

[Alexander Rojas]

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




src/common/http.hpp
Lines 176-183 (patched)


Love this implementation!


- Alexander Rojas


On July 14, 2017, 2:29 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60716/
> ---
> 
> (Updated July 14, 2017, 2:29 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced different authorization-related Acceptor classes with one
> AuthorizationAcceptor class.
> 
> Removed the ObjectAcceptor parent class, since no inheritance features
> are provided by it.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
>   src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
> 
> 
> Diff: https://reviews.apache.org/r/60716/diff/5/
> 
> 
> Testing
> ---
> 
> Passed "make check -j48"
> Passed 'GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60887: Decoupling `Docker::rm` from `Docker::stop` in agent recovery.

[Chun-Hung Hsiao]

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

(Updated July 18, 2017, 5:46 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Addressed Jie's comment.


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


Repository: mesos


Description
---

This patch makes `Docker::stop()` and `Docker::rm()` to explicit calls
during agent recovery (which is the only place using this coupling).
This decoupling avoids a `lambda::bind()` in `Docker::stop()` and thus
enables us to mock `Docker::rm()` in `MockDocker`.


Diffs (updated)
-

  src/docker/docker.hpp 5593cb635e073334c6c2566be3d803cd7febb1c3 
  src/docker/docker.cpp 8081c0203bf62cf62aa3b93d745f0e829ad65509 
  src/slave/containerizer/docker.cpp 2fe92272d7ac6d916371c55affe24598255f10eb 
  src/tests/containerizer/docker_containerizer_tests.cpp 
1e85a79f812399270575ea4a64db10e72f40e648 
  src/tests/mock_docker.hpp 59873646be494c8fe6aebf5ede595d77e3ac4cae 
  src/tests/mock_docker.cpp 0ed63862f5c07935f088282157979eafdc814084 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 60900: Updated Python linter to work with multiple directories.

[Eric Chung]

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


Ship it!




Ship It!

- Eric Chung


On July 18, 2017, 5:31 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60900/
> ---
> 
> (Updated July 18, 2017, 5:31 a.m.)
> 
> 
> Review request for mesos, Eric Chung and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to have different Pylint configurations
> depending on the directory we lint. This will be useful
> in the future to lint our standard Python packages and
> the list of scripts in support/ differently.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/pylint.config  
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60900/diff/2/
> 
> 
> Testing
> ---
> 
> Updated a file in src/python/cli_new and checked that the linter was working 
> as before.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60400: Skipped consulting registry if the agent is in the `slaves.recovered`.

[Jiang Yan Xu]

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

(Updated July 18, 2017, 9:20 a.m.)


Review request for mesos, Benjamin Mahler, James Peach, and Neil Conway.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Skipped consulting registry if the agent is in the `slaves.recovered`.


Diffs (updated)
-

  src/master/master.cpp d895154c00eef17511c46ddd0b75922f952707eb 


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

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


Testing
---

make check.

Will add more tests.


Thanks,

Jiang Yan Xu

Re: Review Request 60820: Added class definition for SlaveID, ContainerID acceptors.

[Greg Mann]

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




src/common/http.hpp
Lines 197-198 (original), 197-211 (patched)


Could you put all of these `XXXIDAcceptor` declarations and definitions in 
alphabetical order?


- Greg Mann


On July 14, 2017, 12:10 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60820/
> ---
> 
> (Updated July 14, 2017, 12:10 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit contains the class definitions for SlaveIDAcceptor and
> ContainerIDAcceptor, which are used to filter the
> '/master/frameworks', '/master/slaves' and '/slave/containers'
> endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93c9b2e58600189867b85175fe4de2dc2f6bf33e 
>   src/common/http.cpp 7dce4cdc82a64702431bbc4307757ec797cf4309 
> 
> 
> Diff: https://reviews.apache.org/r/60820/diff/7/
> 
> 
> Testing
> ---
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60917: Used a real UPID to send `LearnedMessage`.

[Jiang Yan Xu]

> On July 17, 2017, 5:37 p.m., Joseph Wu wrote:
> > src/log/network.hpp
> > Line 247 (original), 248-250 (patched)
> > 
> >
> > I'd prefer this comment to be a `NOTE: ...`
> > 
> > :)

Sure :)


- Jiang Yan


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


On July 17, 2017, noon, Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60917/
> ---
> 
> (Updated July 17, 2017, noon)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-7753
> https://issues.apache.org/jira/browse/MESOS-7753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously an empty UPID `@0.0.0.0:0` was used which could result in
> the message being dropped due to certain security check (MESOS-7401).
> 
> 
> Diffs
> -
> 
>   src/log/network.hpp 0afcfae27394527f7c957e31014cc9a32a4bf63b 
> 
> 
> Diff: https://reviews.apache.org/r/60917/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60925: Updated CHANGELOG for changed semantic of recovery.

[Jiang Yan Xu]

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


Ship it!




Committing with some minor edits.

- Jiang Yan Xu


On July 17, 2017, 5:35 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60925/
> ---
> 
> (Updated July 17, 2017, 5:35 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CHANGELOG for changed semantic of recovery.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 978747fc11c90a0d73d6815823474f974bc0c82e 
>   docs/agent-recovery.md 1b3f2d2d2e630478a8d577eaa6474e3aedc3cb89 
>   docs/upgrades.md dda55f90e2716e805fb145a369fe248cc178f5ac 
> 
> 
> Diff: https://reviews.apache.org/r/60925/diff/2/
> 
> 
> Testing
> ---
> 
> Testing done, renders correctly.
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 60235: Linted support/test-upgrade.py.

[Armand Grillet]

> On July 17, 2017, 11:30 p.m., Joseph Wu wrote:
> > support/test-upgrade.py
> > Lines 114-115 (original), 142-147 (patched)
> > 
> >
> > This refactoring doesn't retain the original logic.
> > 
> > The script does the following:
> > 
> > 1) Launch the Master/Agent/Framework of version A.
> > 2) Kill Agent A, kill Master A, start Master B, start Agent A, start 
> > Framework A.  The agent is only restarted here due to how the 
> > StandaloneMasterDetector is implemented.
> > 3) Kill Agent A, start Agent B, start Framework A.
> > 4) Start Framework B.
> > 
> > This refactoring will cause the master to be killed twice more than the 
> > original and the agent to be killed once more.
> > 
> > I think it would be fine to make 4 new methods for each of the test 
> > steps (start_cluster, upgrade_master, upgrade_agent, upgrade_framework).

I should have documented the change, this is the result of a discussion with 
Benjamin about the fact that these are not test cases if they are not 
independent. As it is not linting anymore I am thinking abour renaming the 
review request "Refactored support/test-upgrade.py.", would you rather suggest 
a linting in that review request with an other one doing the change in the 
logic?


- Armand


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


On June 26, 2017, 3:48 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60235/
> ---
> 
> (Updated June 26, 2017, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/60235/diff/2/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

[Qian Zhang]

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




src/linux/cgroups.hpp
Lines 422 (patched)


Suggest to separte this to 3 lines:
```
{
  return value == that.value;
}
```



src/linux/cgroups.hpp
Lines 425 (patched)


Ditto.



src/linux/cgroups.hpp
Lines 427 (patched)


Why do we need this?



src/linux/cgroups.hpp
Lines 429 (patched)


I think there is no need to have a second `public:` in this class.



src/linux/cgroups.hpp
Lines 446-462 (patched)


Can we merge these 2 structures into one by making the field `Operation op` 
optional? And we could also merge `read_entries()` and `read_stat_entries()` 
into one since they are pretty similar. And merge `Entry::parse()` and 
`StatEntry::parse()` into one by adding another parameter to indict whether 
parsing operation or not.



src/linux/cgroups.hpp
Lines 457 (patched)


Why is this an optional field? Any chance there is no device in an entry?



src/linux/cgroups.hpp
Lines 488 (patched)


s/blkio requests/BIOS requests/



src/linux/cgroups.hpp
Lines 495 (patched)


Ditto.



src/linux/cgroups.hpp
Lines 545-546 (patched)


I see you mentioned `along with devices and types of operations` only for 
this method but not for others, maybe we should mention it for all methods 
which returns `Try`?



src/linux/cgroups.hpp
Lines 575 (patched)


Do you want to put all the methods above into a namespace (e.g., `namespace 
CFQ`)? That seems more aligned with the protobuf messages that you introduced 
in the previous patch.

Or you could delete `message CFQ {` from the previous patch, that seems 
more consistent with the structure of blkio cgroup (blkio.xxx and 
blkio.throttle.)



src/linux/cgroups.hpp
Lines 575-576 (patched)


Merge into a single line.



src/linux/cgroups.cpp
Lines 1939 (patched)


Kill this empty line.



src/linux/cgroups.cpp
Lines 1944 (patched)


Should we replace `unsigned int` with `dev_t` like the code below?
https://github.com/apache/mesos/blob/1.3.0/src/linux/cgroups.cpp#L2572

Or actually I think the above code seems wrong, we should change it from 
`dev_t` to `unsigned int`.



src/linux/cgroups.cpp
Lines 1946 (patched)


Suggest to change this message to:
```
"Invalid device major number: '" + device[0] + "'"
```



src/linux/cgroups.cpp
Lines 1951 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2035 (patched)


Kill this empty line.



src/linux/cgroups.cpp
Lines 2044 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2063 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2072 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2261-2262 (patched)


Merge into a single line.


- Qian Zhang


On July 18, 2017, 8:44 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> ---
> 
> (Updated July 18, 2017, 8:44 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 
> Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Data structure for Blkio entities
> - Stats helpers for blkio.throttle.io* (generic blkio stats)
> - Stats helpers for blkio.io* (CFQ related stats)
> - Comments from the kernel blkio doc for helper functions
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
>   src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 
> 
> 
> Diff: https://reviews.apache.org/r/60933/diff/1/
> 
> 
> Testing
> ---
> 
> make 

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 18, 2017, 6:31 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2886 (patched)
> > 
> >
> > Just curious, the reason that this is a `repeated` field is there are 5 
> > operations (total/read/write/sync/async)?

Yea, per device there are 5 entries in the blkio.io_serviced file.


- Gilbert


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


On July 17, 2017, 5:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 17, 2017, 5:44 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 
> Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 18, 2017, 6:31 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2950 (patched)
> > 
> >
> > Why do we need this msg? I do not see it is used anywhere.

This was left by Jason. Should be removed in favor of:

```
message Statistics {
  repeated CFQ.Statistics cfq = 1;
  repeated CFQ.Statistics cfq_recursive = 2;
  repeated Throttling.Statistics throttling = 3;
}
```

Thanks, Qian!


> On July 18, 2017, 6:31 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2955 (patched)
> > 
> >
> > If we introduce a field like this, does that mean we will fill it in 
> > `BlkioSubsystem::status()` since `CgroupInfo` is part of `ContainerStatus`?

I would suggest let's remove this field and leave a TODO here, which will be 
addressed when we introduce the `prepare` and `status` functions. What do you 
think?


- Gilbert


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


On July 17, 2017, 5:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 17, 2017, 5:44 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 
> Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

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




include/mesos/mesos.proto
Lines 2886 (patched)


Just curious, the reason that this is a `repeated` field is there are 5 
operations (total/read/write/sync/async)?



include/mesos/mesos.proto
Lines 2950 (patched)


Why do we need this msg? I do not see it is used anywhere.



include/mesos/mesos.proto
Lines 2955 (patched)


If we introduce a field like this, does that mean we will fill it in 
`BlkioSubsystem::status()` since `CgroupInfo` is part of `ContainerStatus`?


- Qian Zhang


On July 18, 2017, 8:44 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 18, 2017, 8:44 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 
> Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

[Avinash sridharan]

> On July 17, 2017, 11:12 p.m., Avinash sridharan wrote:
> > src/messages/flags.proto
> > Lines 56 (patched)
> > 
> >
> > Had an internal discussion on this with Jie, and seems like having 
> > different DNS options for CNI and CNM is not something we need to worry 
> > about, so we should merge the fields into one.
> 
> Qian Zhang wrote:
> OK, so the protobuf message should be like the below, right?
> ```
> message ContainerDNS {
>   message DNSInfo {
> // Specify CNI network name or CNM network name as the value of
> // of this field. For CNM host network, its name is `host`, for
> // CNM default bridge network, its name is `bridge`, for a CNM
> // user-defined network, its name is specified by:
> // `ContainerInfo.network_infos(0).name`.
> required string network = 1;
> 
> // For CNI network, all four fields in `slave.cni.spec.DNS` are
> // supported, but for CNM network, we only support three fields:
> // `nameservers`, `search` and `options` but not `domain` since
> // Docker only has `--dns`, `--dns-search`, `--dns-option` options.
> required slave.cni.spec.DNS dns = 2;
>   }
> 
>   repeated DNSInfo dns = 1;
> }
> ```

That looks about right. Another thing that came out of the discussion is that 
we should probably make the `network` field optional. This would then follow 
the semantic of the `network/cni` isolator for missing `network` field implying 
host networks. That said, currently, we should not be supporting setting DNS 
for the host networking since the behavior is different for CNI and CNM. Also, 
changing DNS for host networking doesn't make that much sense. To protect 
against host networking we can add a validation check as a lambda in the 
`--default_container_dns` flag? 

For any unsupported DNS options (`domain` in CNM) we should throw a 
`LOG(WARNING)` while configuring the container's DNS.


- Avinash


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


On July 5, 2017, 7:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated July 5, 2017, 7:03 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp c6803eb54e09a5497755e1e5fef2872193eacba6 
>   src/slave/flags.cpp 398768656b5fa3b7c85474de2b4b008bf7b85cb3 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60915: Enabled filtering of reservations in the agent.

[Alexander Rojas]

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



Looks good to me but it lacks an unit test.

- Alexander Rojas


On July 18, 2017, 11 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60915/
> ---
> 
> (Updated July 18, 2017, 11 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support of the `VIEW_ROLE` ACL to the results generated by the
> `/state` endpoint in the agent for fields `reserved_resources_full`,
> `reserved_resources` and `reserved_resources_allocated`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 60640e5dbf6c3f8c709351e77ca89f287cf12fc3 
> 
> 
> Diff: https://reviews.apache.org/r/60915/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60913: Adds support for OpenSSL's ECDH handshake.

[Alexander Rojas]

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

(Updated July 18, 2017, 2:13 p.m.)


Review request for mesos, Jie Yu and Till Toenshoff.


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


Repository: mesos


Description
---

Support for Elliptic Curve Diffie Hellman algorithm requires extra
configuration parameters which weren't part of Mesos.

This patch enables the extra configuration to Mesos in order to
support ECDH algorithm, it also adds the ssl flag
`LIBPROCESS_SSL_ECDH_CURVES` which allows for the specification of
a specific elliptic curve.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
  3rdparty/libprocess/src/openssl.cpp e6f17e4591f573186e1dc9697e1e7b60a841fe4f 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
8a14dcb865dfab34fb4d0d51f42a28a913fb7ace 


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

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


Testing (updated)
---

```shell
make check
```

Launched Mesos with only ECDHE handshake ciphers enabled

```shell
LIBPROCESS_SSL_ENABLED=1 \
LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
LIBPROCESS_SSL_CIPHERS="ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA"
 \
./bin/mesos-master.sh \
--work_dir=/tmp/mesos/master \
--log_dir=/tmp/mesos/master/log
```

Then in another shell:

```shell
http -v --verify=no https://${MESOS_MASTER_IP}:5050/state

# Launches a browser.
open https://${MESOS_MASTER_IP}:5050/state

# List the set of supported ciphers.
# Expected output:
# >  Starting Nmap 7.50 ( https://nmap.org ) at 2017-07-18 11:41 CEST
# >  Nmap scan report for ${MESOS_MASTER_HOSTNAME} (${MESOS_MASTER_IP})
# >  Host is up (0.13s latency).
# >  rDNS record for ${MESOS_MASTER_IP}: ${MESOS_MASTER_HOSTNAME}
# >  
# >  PORT STATE SERVICE
# >  5050/tcp open  mmcc
# >  | ssl-enum-ciphers:
# >  |   TLSv1.2:
# >  | ciphers:
# >  |   TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
# >  | compressors:
# >  |   NULL
# >  | cipher preference: server
# >  |_  least strength: A
# >  
# >  Nmap done: 1 IP address (1 host up) scanned in 1.87 seconds
wget https://svn.nmap.org/nmap/scripts/ssl-enum-ciphers.nse
nmap --script ssl-enum-ciphers.nse -p 5050 ${MESOS_MASTER_IP}
```


Thanks,

Alexander Rojas