Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

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

(Updated Sept. 21, 2017, 4:27 a.m.)


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


Repository: mesos


Description
---

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.


Diffs (updated)
-

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 


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

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


Testing
---

install tox
cd src/python/lib
tox


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

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

(Updated Sept. 21, 2017, 4:23 a.m.)


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


Repository: mesos


Description (updated)
---

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.


Diffs (updated)
-

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 


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

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


Testing
---

install tox
cd src/python/lib
tox


Thanks,

Eric Chung

Re: Review Request 62447: Revert usage of `-isystem` flag.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [62447, 62446, 62445, 62444, 62161, 62160]

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

Error:
2017-09-21 04:20:18 URL:https://reviews.apache.org/r/62161/diff/raw/ [993/993] 
-> "62161.patch" [1]
error: missing binary patch data for '3rdparty/boost-1.65.0.tar.gz'
error: binary patch does not apply to '3rdparty/boost-1.65.0.tar.gz'
error: 3rdparty/boost-1.65.0.tar.gz: patch does not apply

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

- Mesos Reviewbot


On Sept. 20, 2017, 5:08 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62447/
> ---
> 
> (Updated Sept. 20, 2017, 5:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag break the build of mesos against system libraries
> installed under /usr, because it generates a command line
> of `-isystem /usr/include`, which is explicitly not supported
> by gcc. See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70129
> 
> 
> Diffs
> -
> 
>   configure.ac 92bc1aa5f9604e3b2b678225a57622cd2eb8679a 
> 
> 
> Diff: https://reviews.apache.org/r/62447/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 61174: Added a test `ProtobufTest.ParseJSONOptionalEnum`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 61174 was successfully built and tested.

Reviews applied: `['61109', '61174']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/61174

- Mesos Reviewbot Windows


On Sept. 21, 2017, 1:59 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61174/
> ---
> 
> (Updated Sept. 21, 2017, 1:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ProtobufTest.ParseJSONOptionalEnum`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.cpp 
> 8877e8934e0f7875bfedcfa88b491ce4b13ca44f 
>   3rdparty/stout/tests/protobuf_tests.proto 
> d16726aa8060aea2b830040b20dbdd467c801483 
> 
> 
> Diff: https://reviews.apache.org/r/61174/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 62434: Fixed horizontal ellipsis in Web UI.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Sept. 20, 2017, 10:32 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62434/
> ---
> 
> (Updated Sept. 20, 2017, 10:32 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7978
> https://issues.apache.org/jira/browse/MESOS-7978
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We were previously using non-printable characters which 
> is an issue for the JavaScript linter we plan to use.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/app.js e899452d59663deac9d0166974844710c44cefce 
> 
> 
> Diff: https://reviews.apache.org/r/62434/diff/1/
> 
> 
> Testing
> ---
> 
> Change in the UI tested with Google Chrome 61.0.3163.91.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62434: Fixed horizontal ellipsis in Web UI.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62434]

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

- Mesos Reviewbot


On Sept. 20, 2017, 10:32 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62434/
> ---
> 
> (Updated Sept. 20, 2017, 10:32 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7978
> https://issues.apache.org/jira/browse/MESOS-7978
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We were previously using non-printable characters which 
> is an issue for the JavaScript linter we plan to use.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/app.js e899452d59663deac9d0166974844710c44cefce 
> 
> 
> Diff: https://reviews.apache.org/r/62434/diff/1/
> 
> 
> Testing
> ---
> 
> Change in the UI tested with Google Chrome 61.0.3163.91.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61174: Added a test `ProtobufTest.ParseJSONOptionalEnum`.

[Qian Zhang]

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

(Updated Sept. 21, 2017, 9:59 a.m.)


Review request for mesos, Benjamin Mahler and James Peach.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a test `ProtobufTest.ParseJSONOptionalEnum`.


Diffs (updated)
-

  3rdparty/stout/tests/protobuf_tests.cpp 
8877e8934e0f7875bfedcfa88b491ce4b13ca44f 
  3rdparty/stout/tests/protobuf_tests.proto 
d16726aa8060aea2b830040b20dbdd467c801483 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 61109: Used the default value when parsing an optional enum field from JSON.

[Qian Zhang]

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

(Updated Sept. 21, 2017, 9:58 a.m.)


Review request for mesos, Benjamin Mahler and James Peach.


Changes
---

Rebased and updated the commit message.


Summary (updated)
-

Used the default value when parsing an optional enum field from JSON.


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


Repository: mesos


Description (updated)
---

Previously in MESOS-4997, we have made any inexistent enum value will
be parsed to the default enum value when parsing protobuf message from
a serialized string. Now in this patch, I made parsing protobuf message
from a JSON have the same behavior.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 


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

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


Testing
---

With this patch, when accessing master endpoint with an inexistent enum `xxx` 
in a JSON:
```
curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 
127.0.0.1:5050/api/v1
```
The master log will be:
```
I0725 23:09:53.097790   665 http.cpp:1133] HTTP POST for /master/api/v1 from 
127.0.0.1:49566 with User-Agent='curl/7.47.0'
I0725 23:09:53.098006   665 http.cpp:669] Processing call UNKNOWN
```
This proves when parsing an inexistent enum the default enum value (i.e., 
`UNKNOWN`) will be used.


Thanks,

Qian Zhang

Re: Review Request 61109: Used the default value when parsing an optional enum field.

[Qian Zhang]

> On Sept. 9, 2017, 7:59 a.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert 
> > all invalid enum names into the default value, but AFAICT that is 
> > unavoidable.
> 
> Benjamin Mahler wrote:
> Since we're talking about optional enums, it's not obvious to me whether 
> it's better to leave it unset or to set it to the default. With a required 
> enum, we can't leave it unset so it seems like the default value makes the 
> most sense. However, shouldn't the caller specify the behavior they want? 
> Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? 
> This would be something like `use_default_for_unknown_enum_values`?
> 
> Qian Zhang wrote:
> @Ben, the problem is when `Content-Type` is `application/x-protobuf`, our 
> current implementation is an inexistent enum value will be parsed to the 
> default enum value (i.e., `UNKNOWN`), that is what we have done in 
> MESOS-4997, but when `Content-Type` is `application/json`, the current 
> behavior is different: when parsing an inexistent enum value, we will get an 
> error like `Failed to find enum for 'xxx'` rather than parsing it to the 
> default enum value. So in this patch, I just want to make the two protocols 
> (`application/x-protobuf` and `application/json`) have consistent behavior.
> 
> Benjamin Mahler wrote:
> I see, so this is aiming to make it consistent:
> 
> (1) protobuf: unknown enum value -> set to default
> (2) json before this change: unknown enum value -> error
> (3) json after this change: unknown enum value -> set to default
> 
> When you say "our implementation" for (1), are you referring to what the 
> protobuf parsing functions are doing? Or something that we implemented? If 
> it's the former, then this change sounds good to me, since we're just 
> mimicking the protobuf library parsing behavior in JSON.

> When you say "our implementation" for (1), are you referring to what the 
> protobuf parsing functions are doing? Or something that we implemented?

I am referring to the work that we have done in MESOS-4997, i.e., always use 
optional enum field and include an UNKNOWN value as the first entry in the enum 
list, that way any inexistent enum value will be parsed to the default enum 
value (i.e., UNKNOWN). However, I did not figure out an easy way to verify it, 
I just infer this behavior based on the description in MESOS-4997.


- Qian


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


On July 25, 2017, 11:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> ---
> 
> (Updated July 25, 2017, 11:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used the default value when parsing an optional enum field.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/61109/diff/1/
> 
> 
> Testing
> ---
> 
> With this patch, when accessing master endpoint with an inexistent enum `xxx` 
> in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 
> 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790   665 http.cpp:1133] HTTP POST for /master/api/v1 from 
> 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006   665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., 
> `UNKNOWN`) will be used.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 62418: Removed unneeded configure step in mesos-tidy Docker image.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62417, 62418]

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

- Mesos Reviewbot


On Sept. 19, 2017, 8:42 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62418/
> ---
> 
> (Updated Sept. 19, 2017, 8:42 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The mesos-tidy Docker image uses the cmake build to prepare Mesos for
> linting. The cmake build does not require running of 'bootstrap'.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh 68026f4a00abbca7fc80688b9d16718440596632 
> 
> 
> Diff: https://reviews.apache.org/r/62418/diff/1/
> 
> 
> Testing
> ---
> 
> Ran the built image with a modified `support/mesos-tidy.sh` that does not 
> force-download the image.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 62176: Added cmake dependency check for libsasl2 on non-Windows platforms.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62105, 62106, 62176]

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

- Mesos Reviewbot


On Sept. 19, 2017, 4:58 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62176/
> ---
> 
> (Updated Sept. 19, 2017, 4:58 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, Greg Mann, 
> Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3110
> https://issues.apache.org/jira/browse/MESOS-3110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added cmake dependency check for libsasl2 on non-Windows platforms.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7 
> 
> 
> Diff: https://reviews.apache.org/r/62176/diff/3/
> 
> 
> Testing
> ---
> 
> I tested this on my Ubuntu 16.04 system. When the libsasl2 library doesn't 
> exist, it fails the cmake configure/build.  I did not test this on any other 
> platform, but this code is in a "if (NOT WIN32)" block and won't affect a 
> Windows build.  I'm uncertain if there is much support for other kinds of 
> builds (like Mac OS), but this should be platform independent.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 62443: Initialized tests with Google Mock initialization.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Sept. 20, 2017, 11:13 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62443/
> ---
> 
> (Updated Sept. 20, 2017, 11:13 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7987
> https://issues.apache.org/jira/browse/MESOS-7987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switched over to explicitly using Google Mock to initialize the test
> suite. This enables Google Mock option flags so that --gmock_verbose
> will be supported.
> 
> 
> Diffs
> -
> 
>   src/tests/main.cpp cd30cac89c8d676d60a90f0ef843a68c241a0431 
> 
> 
> Diff: https://reviews.apache.org/r/62443/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> Manually verified that `--gmock_verbose=info` traces the mocked expectations.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62353: Added a master-registry backed resource provider manager registry.

[Jie Yu]

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




src/master/registry.proto
Lines 116 (patched)


indentation?



src/resource_provider/registrar.hpp
Lines 131 (patched)


tailing `{` should be in a new line



src/resource_provider/registrar.hpp
Lines 133 (patched)


No need for `explicit` because this is a two arguments constructor



src/resource_provider/registrar.hpp
Lines 144 (patched)


nits: no need for tailing underscore to be consistent with other files.



src/resource_provider/registrar.cpp
Lines 382 (patched)


Ditto. No need for `explicit`



src/resource_provider/registrar.hpp
Lines 134 (patched)


Move the tailing `{` to the next line.



src/resource_provider/registrar.hpp
Lines 147 (patched)


Ditto. Remove tailing underscore



src/resource_provider/registrar.cpp
Lines 372-376 (patched)


This is definitely weird to me. I think we can get rid of the dependency of 
taking an internal `internal::Registry& registry` by doing the following:

1) Move `AdaptedOperation` to master registrar. I would call it 
`UpdateResourceProviderRegistryOperation`.

2) `recover()` probably does not need to return `Registry`. If we get rid 
of that, we probably don't need to expose the internal Registry.


- Jie Yu


On Sept. 19, 2017, 4:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62353/
> ---
> 
> (Updated Sept. 19, 2017, 4:54 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an implementation of the resource provider registrar
> backed by the master's registrar. With that it becomes possible to
> persist resource provider manager state in a single master registrar,
> but use the narrowly defined resource provider registry.
> 
> 
> Diffs
> -
> 
>   src/master/registry.proto 362a9fab946e9fb0411df2277f7f3edbadccb61a 
>   src/resource_provider/registrar.hpp PRE-CREATION 
>   src/resource_provider/registrar.cpp PRE-CREATION 
>   src/tests/resource_provider_manager_tests.cpp 
> 3bc56b51526e9dd188423f7349e74246c3295c77 
> 
> 
> Diff: https://reviews.apache.org/r/62353/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when 'ResourceProviderManager' updates.

[Benjamin Bannier]

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




src/slave/slave.cpp
Lines 1272-1285 (patched)


This might be redundant depending on when we are able to instantiate the 
resource provider manager.

In another patch I make creating of the registrar dependent on the 
`SlaveID` which would force us to delay creating of the manager (which depends 
on the registrar) to somewhere here. We would in that case not yet have seen 
any updates.


- Benjamin Bannier


On Sept. 20, 2017, 4:24 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> ---
> 
> (Updated Sept. 20, 2017, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
> 
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
> 
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
> 
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
> 
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521 
>   src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef 
>   src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82 
>   src/tests/oversubscription_tests.cpp 
> 02b10d6689dd6f01510cd2d5db2bb76b4b190eca 
>   src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723 
> 
> 
> Diff: https://reviews.apache.org/r/61183/diff/10/
> 
> 
> Testing
> ---
> 
> Tested on a number of platforms on internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 62252: Added `process::Executor::execute()`.

[Benjamin Hindman]

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




3rdparty/libprocess/include/process/executor.hpp
Lines 71 (patched)


Let's use the `std::enable_if<..., int>::type = 0` for now since that's 
what we've been trying to converge on other places. Here and below, thanks!



3rdparty/libprocess/include/process/executor.hpp
Lines 87 (patched)


Can we also add a TODO that we ultimately want to capture `f` via a 
forward/move? Thanks!

Can we do this?

`std::bind([](F&& f) { f(); return Nothing(); }, std::forward(f));`



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1242 (patched)


Can we update the code to use `CountDownLatch` so we can use 
`AWAIT_READY(latch.triggered())` and also pull all the test chunks together 
"locally" so that it's easier to see what each piece does. Thanks!


- Benjamin Hindman


On Sept. 15, 2017, 3:17 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62252/
> ---
> 
> (Updated Sept. 15, 2017, 3:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7970
> https://issues.apache.org/jira/browse/MESOS-7970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a convenient interface to `process::Executor` to
> asynchronously execute arbitrary functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/executor.hpp 
> cd2f2f03cba8a435f142b31def9f89a6cd61af73 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 82efb2f8449e4cb13620cae1a49321fc42e2db60 
> 
> 
> Diff: https://reviews.apache.org/r/62252/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62443: Initialized tests with Google Mock initialization.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Sept. 20, 2017, 6:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62443/
> ---
> 
> (Updated Sept. 20, 2017, 6:13 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7987
> https://issues.apache.org/jira/browse/MESOS-7987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switched over to explicitly using Google Mock to initialize the test
> suite. This enables Google Mock option flags so that --gmock_verbose
> will be supported.
> 
> 
> Diffs
> -
> 
>   src/tests/main.cpp cd30cac89c8d676d60a90f0ef843a68c241a0431 
> 
> 
> Diff: https://reviews.apache.org/r/62443/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> Manually verified that `--gmock_verbose=info` traces the mocked expectations.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61109: Used the default value when parsing an optional enum field.

[Benjamin Mahler]

> On Sept. 8, 2017, 11:59 p.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert 
> > all invalid enum names into the default value, but AFAICT that is 
> > unavoidable.
> 
> Benjamin Mahler wrote:
> Since we're talking about optional enums, it's not obvious to me whether 
> it's better to leave it unset or to set it to the default. With a required 
> enum, we can't leave it unset so it seems like the default value makes the 
> most sense. However, shouldn't the caller specify the behavior they want? 
> Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? 
> This would be something like `use_default_for_unknown_enum_values`?
> 
> Qian Zhang wrote:
> @Ben, the problem is when `Content-Type` is `application/x-protobuf`, our 
> current implementation is an inexistent enum value will be parsed to the 
> default enum value (i.e., `UNKNOWN`), that is what we have done in 
> MESOS-4997, but when `Content-Type` is `application/json`, the current 
> behavior is different: when parsing an inexistent enum value, we will get an 
> error like `Failed to find enum for 'xxx'` rather than parsing it to the 
> default enum value. So in this patch, I just want to make the two protocols 
> (`application/x-protobuf` and `application/json`) have consistent behavior.

I see, so this is aiming to make it consistent:

(1) protobuf: unknown enum value -> set to default
(2) json before this change: unknown enum value -> error
(3) json after this change: unknown enum value -> set to default

When you say "our implementation" for (1), are you referring to what the 
protobuf parsing functions are doing? Or something that we implemented? If it's 
the former, then this change sounds good to me, since we're just mimicking the 
protobuf library parsing behavior in JSON.


- Benjamin


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


On July 25, 2017, 3:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> ---
> 
> (Updated July 25, 2017, 3:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used the default value when parsing an optional enum field.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/61109/diff/1/
> 
> 
> Testing
> ---
> 
> With this patch, when accessing master endpoint with an inexistent enum `xxx` 
> in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 
> 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790   665 http.cpp:1133] HTTP POST for /master/api/v1 from 
> 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006   665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., 
> `UNKNOWN`) will be used.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 62447: Revert usage of `-isystem` flag.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 62161.

Failed command: `python.exe .\support\apply-reviews.py -n -r 62161`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62447

Relevant logs:

- 
[apply-review-62161-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62447/logs/apply-review-62161-stdout.log):

```
error: missing binary patch data for '3rdparty/boost-1.65.0.tar.gz'
error: binary patch does not apply to '3rdparty/boost-1.65.0.tar.gz'
error: 3rdparty/boost-1.65.0.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On Sept. 20, 2017, 5:08 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62447/
> ---
> 
> (Updated Sept. 20, 2017, 5:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag break the build of mesos against system libraries
> installed under /usr, because it generates a command line
> of `-isystem /usr/include`, which is explicitly not supported
> by gcc. See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70129
> 
> 
> Diffs
> -
> 
>   configure.ac 92bc1aa5f9604e3b2b678225a57622cd2eb8679a 
> 
> 
> Diff: https://reviews.apache.org/r/62447/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 62447: Revert usage of `-isystem` flag.

[Andrew Schwartzmeyer]

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



It looks like the problem is `-isystem /usr/include` according to the upstream 
bug report, but all of these paths should be of the form (just looking over 
them) `-isystem /usr/include/foo` or `-isystem /blah/foo/include` and not just 
raw `/usr/include`. Which is the problematic one?

- Andrew Schwartzmeyer


On Sept. 20, 2017, 10:08 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62447/
> ---
> 
> (Updated Sept. 20, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag break the build of mesos against system libraries
> installed under /usr, because it generates a command line
> of `-isystem /usr/include`, which is explicitly not supported
> by gcc. See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70129
> 
> 
> Diffs
> -
> 
>   configure.ac 92bc1aa5f9604e3b2b678225a57622cd2eb8679a 
> 
> 
> Diff: https://reviews.apache.org/r/62447/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 62446: Additional check when building with java bindings enabled.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

Check whether the versions of libprotobuf used by mesos
and by maven are different, which will lead to a
compile-time failure that may be hard to understand
for those not familiar with maven.


Diffs
-

  configure.ac 92bc1aa5f9604e3b2b678225a57622cd2eb8679a 


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


Testing
---


Thanks,

Benno Evers

Review Request 62447: Revert usage of `-isystem` flag.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

This flag break the build of mesos against system libraries
installed under /usr, because it generates a command line
of `-isystem /usr/include`, which is explicitly not supported
by gcc. See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70129


Diffs
-

  configure.ac 92bc1aa5f9604e3b2b678225a57622cd2eb8679a 


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


Testing
---


Thanks,

Benno Evers

Review Request 62445: Remove duplicate block in configure.ac.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

This blocks seems to have been copy/pasted from another place.


Diffs
-

  configure.ac 92bc1aa5f9604e3b2b678225a57622cd2eb8679a 


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


Testing
---


Thanks,

Benno Evers

Review Request 62444: Add UNREACHABLE() macro to __cxa_pure_virtual.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

The function __cxa_pure_virtual must not return,
but newer versions of clang detect that the expansion
of the RAW_LOG() macro contains returning code paths
for arguments other than FATAL.


Diffs
-

  src/logging/logging.cpp 70d66a5c396f709e8f27ad0d51315ed6d257f73b 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 62160: Fix stout build with newer boost versions.

[Benno Evers]

> On Sept. 19, 2017, 7:39 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/tests/json_tests.cpp
> > Lines 358-359 (original)
> > 
> >
> > Since the code here was weird even before, it seems that we should 
> > change `JSON::True` and `JSON::False` to be factories of `JSON::Boolean`, 
> > e.g.,
> > 
> > Boolean False()
> > {
> >   return {false};
> > }
> > 
> > Boolean True()
> > {
> >   return {true};
> > }
> > 
> > With such factories we could keep users of `Boolean` as brief as they 
> > are now, and not resort to slicing as a usual measure. This change would 
> > also be in line with what we did when created `Bytest` factories out of 
> > existing subclasses.
> > 
> > Would you be able to create a patch for that?

This will break all code that creates objects of type `JSON::True` or 
`JSON::False`. We could simultaneously fix all uses in mesos/libprocess, but 
technically `stout` is an independent library so I think we should not break 
API compatibility for such a small improvement.


- Benno


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


On Sept. 12, 2017, 11:08 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62160/
> ---
> 
> (Updated Sept. 12, 2017, 11:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Starting from Boost 1.62, Boost.Variant added additional
> compile-time checks to its constructors to fix this
> issue: https://svn.boost.org/trac10/ticket/11602
> 
> However, this breaks some places in stout which try
> to access a derived class from a variant holding the
> base class.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> 4f9ea1b34537026af80edd97fa0b3ae1a3dfa862 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
>   3rdparty/stout/tests/json_tests.cpp 
> adaa343faf3b557ad483675318b22eb90b1eeb52 
> 
> 
> Diff: https://reviews.apache.org/r/62160/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 62443: Initialized tests with Google Mock initialization.

[Mesos Reviewbot Windows]

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



Bad review!

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On Sept. 20, 2017, 4:58 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62443/
> ---
> 
> (Updated Sept. 20, 2017, 4:58 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-7987
> https://issues.apache.org/jira/browse/MESOS-7987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switched over to explicitly using Google Mock to initialize the test
> suite. This enables Google Mock option flags so that --gmock_verbose
> will be supported.
> 
> 
> Diffs
> -
> 
>   src/tests/main.cpp cd30cac89c8d676d60a90f0ef843a68c241a0431 
> 
> 
> Diff: https://reviews.apache.org/r/62443/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> Manually verified that `--gmock_verbose=info` traces the mocked expectations.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 62443: Initialized tests with Google Mock initialization.

[James Peach]

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

Review request for mesos.


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


Repository: mesos


Description
---

Switched over to explicitly using Google Mock to initialize the test
suite. This enables Google Mock option flags so that --gmock_verbose
will be supported.


Diffs
-

  src/tests/main.cpp cd30cac89c8d676d60a90f0ef843a68c241a0431 


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


Testing
---

make check (Fedora 26)

Manually verified that `--gmock_verbose=info` traces the mocked expectations.


Thanks,

James Peach

Re: Review Request 62439: Used a namespace alias for 'process::http'.

[Jan Schlicht]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 155-156 (patched)


In general it seems that we put namespace aliases before using 
declarations, e.g. in `fetcher.cpp`. Please move the alias.


- Jan Schlicht


On Sept. 20, 2017, 4:24 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62439/
> ---
> 
> (Updated Sept. 20, 2017, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces 'http' as an alias for 'process::http'.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82 
> 
> 
> Diff: https://reviews.apache.org/r/62439/diff/1/
> 
> 
> Testing
> ---
> 
> Tested on a number of platforms on internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61528: Implemented a registrar for resource provider manager state.

[Benjamin Bannier]

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

(Updated Sept. 20, 2017, 4:24 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch adds a registry and registrar interface for resource
provider managers. The registrar interface is modelled after the
master registrar and supports similar operations. Currently a single,
LevelDB-backed registrar is implemented which we plan to use for
resource provider managers in agents.

Current the registry allows to add and remove resource provider IDs.


Diffs (updated)
-

  src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
  src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900 
  src/resource_provider/registrar.hpp PRE-CREATION 
  src/resource_provider/registrar.cpp PRE-CREATION 
  src/resource_provider/registry.hpp PRE-CREATION 
  src/resource_provider/registry.proto PRE-CREATION 
  src/slave/paths.hpp d021e6b85a2e5823ea088d65faf9cd85cfb57e28 
  src/slave/paths.cpp e8724bf3e382211b5882728b86575de75981307f 
  src/tests/resource_provider_manager_tests.cpp 
3bc56b51526e9dd188423f7349e74246c3295c77 


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

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


Testing (updated)
---

Tested on a number of platforms on internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when 'ResourceProviderManager' updates.

[Benjamin Bannier]

> On Sept. 19, 2017, 12:03 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 6669 (patched)
> > 
> >
> > add a `break` for the first level switch? and a default?

I added a `break` here.

I did not add a `default` branch as we want to explicitly trigger a compiler 
warning about uncovered cases here should another message type be added in the 
future.


> On Sept. 19, 2017, 12:03 a.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp
> > Lines 8357 (patched)
> > 
> >
> > can we get rid of the process:: prefix?

All types from `process::http` in this file are fully qualified (e.g., also 
`process::http::Request`). I'll submit an additional RR to tidy this up 
(https://reviews.apache.org/r/62439/).


> On Sept. 19, 2017, 12:03 a.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp
> > Lines 8372 (patched)
> > 
> >
> > Do we need the `mesos::` prefix?

I believe sadly we do. We for sure what to distinguish unversioned from e.g., 
`v1` types. On the other hand in tests we have a namespace 
`mesos::internal::v1` and just e.g., `v1::resource_provider::Event::Subscribed` 
is looked up in that one instead of `mesos::v1`.

I guess an alternative would be to define aliases for `v1` types and 
subnamespaces, but I don't see that being done currently (and it IMO seems to 
make things even more convoluted).


- Benjamin


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


On Sept. 20, 2017, 4:24 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> ---
> 
> (Updated Sept. 20, 2017, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent.
> 
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
> 
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
> 
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
> 
> This patch also adjusts a number of tests to no expect two
> 'UpdateSlaveMessage's.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521 
>   src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef 
>   src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82 
>   src/tests/oversubscription_tests.cpp 
> 02b10d6689dd6f01510cd2d5db2bb76b4b190eca 
>   src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723 
> 
> 
> Diff: https://reviews.apache.org/r/61183/diff/10/
> 
> 
> Testing
> ---
> 
> Tested on a number of platforms on internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when 'ResourceProviderManager' updates.

[Benjamin Bannier]

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

(Updated Sept. 20, 2017, 4:24 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Addressed Jie's comments, mainly

* Always send out updated total. This simplifies the agent code a lot and has 
minimal effects as with the created https://reviews.apache.org/r/62438/ the 
master would ignore redundant agent updates. This change requires adjusting a 
few oversubscription tests that were only expecting a single update.
* Improved logging in a few places.


Repository: mesos


Description (updated)
---

The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent.

In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).

Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.

To facilitate logging we add a stringification function for
'ResourceProviderMessage's.

This patch also adjusts a number of tests to no expect two
'UpdateSlaveMessage's.


Diffs (updated)
-

  src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521 
  src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef 
  src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82 
  src/tests/oversubscription_tests.cpp 02b10d6689dd6f01510cd2d5db2bb76b4b190eca 
  src/tests/slave_tests.cpp e9bcfefd52e1e99a7a5877b2e7c30af958ca1723 


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

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


Testing (updated)
---

Tested on a number of platforms on internal CI.


Thanks,

Benjamin Bannier

Review Request 62438: Ignored redundant agent resources updates in master.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


Repository: mesos


Description
---

In the future, agents will send updates on their total, e.g., when
resource providers are added or removed. As an update to the agent's
total resources currently triggers rescinding of all offered agent
resources, spurious updates can negatively affect in-flight offer
operations.

This patch changes the master so that updates introducing no changes
(i.e., the new resources are identical to the old resources) are
dropped and do not trigger rescinding of offers anymore. We also
adjust the handling of oversubscribed agent resources to drop
redundant updates.


Diffs
-

  src/master/master.cpp 6d84a26bff970b842b58dfb69dbf232ba5c16a20 


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


Testing
---

Tested on a number of platforms on internal CI.


Thanks,

Benjamin Bannier

Review Request 62439: Used a namespace alias for 'process::http'.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


Repository: mesos


Description
---

This patch introduces 'http' as an alias for 'process::http'.


Diffs
-

  src/slave/slave.cpp 01020a624bdd67ee6371f607d897ad71a2cdcc82 


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


Testing
---

Tested on a number of platforms on internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 62158: Rescinded offers possibly affected by updates to agent total resources.

[Benjamin Bannier]

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

(Updated Sept. 20, 2017, 4:24 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Added a `break` to logging code.


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


Repository: mesos


Description
---

When an agent changes its resources, the master should rescind any
offers affected by the change. We already performed the rescind for
updates to the agent's oversubscribed resources; this patch adds offer
rescinding when an update an agent's total resources is processed.

While for updates to an agent's oversubscribed resources we currently
only rescind offers containing revocable resources to e.g., reduce
offer churn, we for updates to the total we here currently rescind all
offers for resources on the agent.


Diffs (updated)
-

  src/master/master.cpp 6d84a26bff970b842b58dfb69dbf232ba5c16a20 


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

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


Testing (updated)
---

Tested on a number of platforms on internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 62434: Fixed horizontal ellipsis in Web UI.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 62434 was successfully built and tested.

Reviews applied: `['62434']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62434

- Mesos Reviewbot Windows


On Sept. 20, 2017, 4:02 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62434/
> ---
> 
> (Updated Sept. 20, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7978
> https://issues.apache.org/jira/browse/MESOS-7978
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We were previously using non-printable characters which 
> is an issue for the JavaScript linter we plan to use.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/app.js e899452d59663deac9d0166974844710c44cefce 
> 
> 
> Diff: https://reviews.apache.org/r/62434/diff/1/
> 
> 
> Testing
> ---
> 
> Change in the UI tested with Google Chrome 61.0.3163.91.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 62434: Fixed horizontal ellipsis in Web UI.

[Armand Grillet]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

We were previously using non-printable characters which 
is an issue for the JavaScript linter we plan to use.


Diffs
-

  src/webui/master/static/js/app.js e899452d59663deac9d0166974844710c44cefce 


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


Testing
---

Change in the UI tested with Google Chrome 61.0.3163.91.


Thanks,

Armand Grillet

Re: Review Request 60496: Added socket checking to the network ports isolator.

[Qian Zhang]

> On Sept. 19, 2017, 2:13 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 437 (patched)
> > 
> >
> > I think we need to introduce a new reason 
> > `REASON_CONTAINER_LIMITATION_PORT` and use it here rather than using 
> > `REASON_CONTAINER_LIMITATION`.
> 
> James Peach wrote:
> Why do you think a special port limitation is needed? I rather thought 
> that the `DISK` and `MEMORY` reasons were the anomaly ...
> 
> Qian Zhang wrote:
> I just want it to be consistent with memory and disk, now 
> `REASON_CONTAINER_LIMITATION` is not used anywhere, I am not sure why we need 
> it. Actually I am OK with either way: 1) we remove 
> `REASON_CONTAINER_LIMITATION_MEMORY` and `REASON_CONTAINER_LIMITATION_DISK` 
> and always use `REASON_CONTAINER_LIMITATION` or 2) we use `DISK`, `MEMORY` 
> and `PORT` respectively. For now, I think 2) should be an easier one to go 
> with.
> 
> James Peach wrote:
> External isolators already have to use `REASON_CONTAINER_LIMITATION`. If 
> we add a new (IMHO unnecessary) reason here, then schedulers won't be able to 
> understand the limitation until they upgrade ther protobufs. I don't think 
> that `REASON_CONTAINER_LIMITATION_PORT` is worth it.
> 
> The resource-specific reasons go all the way back to 
> [r/26382](https://reviews.apache.org/r/26382/) when `REASON_MEMORY_LIMIT` was 
> the only limitation that could be raised even though isolator modules did 
> exist at that time.

OK, I agree.


- Qian


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


On Sept. 19, 2017, 8:20 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> ---
> 
> (Updated Sept. 19, 2017, 8:20 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented ports resource restrictions in the network ports isolator.
> Periodically, scan for listening sockets and match them up to all
> the open sockets in the containers we are tracking in the network.
> Check any sockets we find against the ports resource and trigger a
> resource limitation if the port has not been allocated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60496/diff/20/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>