Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On July 9, 2015, 9:19 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2815-2819
> > 
> >
> > Any reason we didn't convert os::stat::mtime to return a Time?
> > 
> > The only other user of os::stat::mtime does the same messy conversion:
> > 
> > ```
> > Future Slave::garbageCollect(const string& path)
> > {
> >   Try mtime = os::stat::mtime(path);
> >   if (mtime.isError()) {
> > LOG(ERROR) << "Failed to find the mtime of '" << path
> ><< "': " << mtime.error();
> > return Failure(mtime.error());
> >   }
> > 
> >   // It is unsafe for testing to use unix time directly, we must use
> >   // Time::create to convert into a Time object that reflects the
> >   // possibly advanced state of the libprocess Clock.
> >   Try time = Time::create(mtime.get());
> >   CHECK_SOME(time);
> > ```
> 
> Till Toenshoff wrote:
> One reason could be that os::stat::mtime lives in stout whereas Time 
> lives in libprocess, no?

Till is right, it is the only reason. `os::stat::mtime` is part of stout while 
`Time` is part of libprocess. We look into moving `Time` to stout, but there is 
a dependency to the clock which forbids the transition.


> On July 9, 2015, 9:19 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2833
> > 
> >
> > Why did you need the {} here?

It initializes a struct to be zero. It is equivalent to do 
`memset(&clientMTime, 0, sizeof(clientMTime))`, although my original patch 
called for using the more standard `= {0}` some previous reviewer suggested to 
change it to the version in the patch.


> On July 9, 2015, 9:19 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2839-2846
> > 
> >
> > Any reason to prefer -1 special cases here to just using Try and 
> > handling errors?

-1 is the value returned in error by `std::mktime`. As mentioned before, we 
couldn't use `Time`. Though it is possible to create a `Time` from a `time_t` 
initialized with `mktime`. I feel it just adds more points of failure.


- Alexander


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


On June 17, 2015, 5:42 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 17, 2015, 5:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When serving a static file, libprocess returns the header `Last-Modified` 
> which is used by browsers to control Cache.
> When a http request arrives containing the header `If-Modified-Since`, a 
> response `304 Not Modified` is returned if the date in the request and the 
> modification time (as returned by doing `stat` in the file) coincide.
> Unit tests added.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> e47cc7afbc8110759edf25a2dc05d09eda25c417 
>   3rdparty/libprocess/src/process.cpp 
> a67a3afdb30d23eb1b265b04ae662f64e874b6c6 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 36547: Fixed fetcher failing for FTP URIs.

[Jan Schlicht]

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

Review request for mesos, Bernd Mathiske and Joerg Schad.


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


Repository: mesos


Description
---

The response code for successful FTP file transfers is 226, while it is 200 for 
HTTP. The fetcher has been changed to check for a response code of 226 for FTP 
URIs.


Diffs
-

  src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 

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


Testing
---

make check & external FTP server test.


Thanks,

Jan Schlicht

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Joerg Schad]

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



src/launcher/fetcher.cpp (line 123)


s\response\status (to be consistent with below)



src/launcher/fetcher.cpp (line 125)


s\successCode\successStatusCode



src/launcher/fetcher.cpp (line 128)


indentation +2 spaces 
(https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals)


- Joerg Schad


On July 16, 2015, 2:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 2:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Joerg Schad]

> On July 16, 2015, 2:37 p.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 128
> > 
> >
> > indentation +2 spaces 
> > (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals)

see line 61 int he same file for an example :-)


- Joerg


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


On July 16, 2015, 2:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 2:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Jan Schlicht]

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

(Updated July 16, 2015, 2:48 p.m.)


Review request for mesos, Bernd Mathiske and Joerg Schad.


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


Repository: mesos


Description
---

The response code for successful FTP file transfers is 226, while it is 200 for 
HTTP. The fetcher has been changed to check for a response code of 226 for FTP 
URIs.


Diffs (updated)
-

  src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 

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


Testing
---

make check & external FTP server test.


Thanks,

Jan Schlicht

Re: Review Request 35702: Added /reserve HTTP endpoint to the master.

[Alexander Rukletsov]

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



src/master/http.cpp (line 447)


Not directly related to endpoints, but to dynamic reservations in general. 
Do you think it makes sense to bookkeep dynamic reservation or have an 
aggregating method in `mesos::internal::master::Role`?


- Alexander Rukletsov


On June 28, 2015, 8:36 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35702/
> ---
> 
> (Updated June 28, 2015, 8:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2600
> https://issues.apache.org/jira/browse/MESOS-2600
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This involved a lot more challenges than I anticipated, I've captured the 
> various approaches and limitations and deal-breakers of those approaches 
> here: [Master Endpoint Implementation 
> Challenges](https://docs.google.com/document/d/1cwVz4aKiCYP9Y4MOwHYZkyaiuEv7fArCye-vPvB2lAI/edit#)
> 
> Key points:
> 
> * This is a stop-gap solution until we shift the offer creation/management 
> logic from the master to the allocator.
> * `updateAvailable` and `updateSlave` are kept separate because
>   (1) `updateAvailable` is allowed to fail whereas `updateSlave` must not.
>   (2) `updateAvailable` returns a `Future` whereas `updateSlave` does not.
>   (3) `updateAvailable` never leaves the allocator in an over-allocated state 
> and must not, whereas `updateSlave` does, and can.
> * The algorithm:
> * Initially, the master pessimistically assume that what seems like 
> "available" resources will be gone.
>   This is due to the race between the allocator scheduling an `allocate` 
> call to itself vs master's `allocator->updateAvailable` invocation.
>   As such, we first try to satisfy the request only with the offered 
> resources.
> * We greedily rescind one offer at a time until we've rescinded 
> sufficiently many offers.
>   IMPORTANT: We perform `recoverResources(..., Filters())` rather than 
> `recoverResources(..., None())` so that we can pretty much always win the 
> race against `allocate`.
>  In the case that we lose, no disaster occurs. We simply fail 
> to satisfy the request.
> * If we still don't have enough resources after resciding all offers, be 
> optimistic and forward the request to the allocator since there may be 
> available resources to satisfy the request.
> * If the allocator returns a failure, report the error to the user with 
> `PreconditionFailed`. This could be updated to be `Forbidden`, or `Conflict` 
> maybe as well. We'll pick one eventually.
> 
> This approach is clearly not ideal, since we would prefer to rescind as 
> little offers as possible.
> The challenges of implementing the ideal solution in the current state is 
> described in the document above.
> 
> TODO(mpark): Add more comments and test cases.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 
>   src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 
>   src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df 
>   src/master/validation.hpp 469d6f56c3de28a34177124aae81ce24cb4ad160 
>   src/master/validation.cpp 9d128aa1b349b018b8e4a1916434d848761ca051 
> 
> Diff: https://reviews.apache.org/r/35702/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36537: Made TaskState.data available via state.json endpoint.

[Kapil Arya]

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

(Updated July 16, 2015, 10:54 a.m.)


Review request for mesos, Benjamin Hindman and Timothy Chen.


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


Repository: mesos


Description
---

This would allows us to expose the docker container IP (that is queried via
docker-inspect and is part of TaskState.data) to Mesos-DNS via
Master state.json endpoint.

Currently, Master doesn't store TaskState::data and so it's not made available 
via state.json. A follow up patch would fix it.


Diffs
-

  src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 

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


Testing
---

Tested by modifying the test_executor to send data with TASK_RUNNING status 
update. The data showed up in Slave's state.json.


Thanks,

Kapil Arya

Re: Review Request 35983: Added /unreserve HTTP endpoint to the master.

[Alexander Rukletsov]

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



src/master/http.cpp (line 1291)


As in https://reviews.apache.org/r/35702/, I suggest we extract validation 
into a separate function.



src/master/http.cpp (lines 1325 - 1332)


Why do we need to recover resources for unreserve?


- Alexander Rukletsov


On June 28, 2015, 8:37 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35983/
> ---
> 
> (Updated June 28, 2015, 8:37 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2600
> https://issues.apache.org/jira/browse/MESOS-2600
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 
>   src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 
>   src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df 
> 
> Diff: https://reviews.apache.org/r/35983/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Bernd Mathiske]

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



src/launcher/fetcher.cpp (line 135)


Suggestion: put the condition check for the protocol AFTER checking if we 
have an error. Then we do not need the comment above nor the extra variable AND 
we can state in the error message which specific protocol is meant instead of 
"HTTP/FTP".


- Bernd Mathiske


On July 16, 2015, 7:48 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 7:48 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36547]

All tests passed.

- Mesos ReviewBot


On July 16, 2015, 2:48 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36537: Made TaskState.data available via state.json endpoint.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36537]

All tests passed.

- Mesos ReviewBot


On July 16, 2015, 2:54 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36537/
> ---
> 
> (Updated July 16, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Timothy Chen.
> 
> 
> Bugs: MESOS-3061
> https://issues.apache.org/jira/browse/MESOS-3061
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This would allows us to expose the docker container IP (that is queried via
> docker-inspect and is part of TaskState.data) to Mesos-DNS via
> Master state.json endpoint.
> 
> Currently, Master doesn't store TaskState::data and so it's not made 
> available via state.json. A follow up patch would fix it.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 
> 
> Diff: https://reviews.apache.org/r/36537/diff/
> 
> 
> Testing
> ---
> 
> Tested by modifying the test_executor to send data with TASK_RUNNING status 
> update. The data showed up in Slave's state.json.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Jan Schlicht]

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

(Updated July 16, 2015, 6:55 p.m.)


Review request for mesos, Bernd Mathiske and Joerg Schad.


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


Repository: mesos


Description
---

The response code for successful FTP file transfers is 226, while it is 200 for 
HTTP. The fetcher has been changed to check for a response code of 226 for FTP 
URIs.


Diffs (updated)
-

  src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 

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


Testing
---

make check & external FTP server test.


Thanks,

Jan Schlicht

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On July 16, 2015, 9:55 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 9:55 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 35715: Added revocable resource state validation.

[Niklas Nielsen]

> On June 21, 2015, 11:47 a.m., Vinod Kone wrote:
> > src/common/resources.cpp, lines 479-487
> > 
> >
> > These checks are done in master's validation.cpp
> 
> Michael Park wrote:
> Ah sorry, I missed that.
> 
> This reminded me of the discussion Jie and I had for 
> [r32140](https://reviews.apache.org/r/32140/) regarding where validations 
> should live. I think this validation belongs here rather than in master 
> validation.
> What we concluded from the discussion was that `Resources::validate` 
> should perform necessary validation to satisfy the invariant of the 
> `Resource` object.
> This enables methods that operate on `Resource` (e.g. 
> `Resources::isRevocable`) to assume its validity.
> 
> My notes:
> > Synced with Jie on IRC regarding this topic. We agreed that 
> `Resources::validate` needs to capture the invariant of the `Resource` object 
> which means it needs to invalidate the `role == "*" && has_reservation()` 
> state. This invariant is required for all the predicates as well as functions 
> such as `reserved()` and `unreserved()` to have well-defined behavior.
> 
> Jie's note:
> > Discussed with Mpark offline. We agreed that rule for 
> Resources::validate is that it should only perform necessary validation to 
> make sure all methods in Resources are well hahaved, and the validation 
> around * and reservation info is necessary for 'reserved/unreserved' to work 
> properly. Thus dropping the issue around validation.
> 
> Michael Park wrote:
> I found Jie's comment regarding this: 
> https://reviews.apache.org/r/33865/#comment133597
> 
> @Jie: My thought here was that these checks are necessary to make 
> `isRevocable` well-defined. The same way the check for `"*" resource cannot 
> be dynamically reserved` is necessary to make `isDynamicallyReserved` and 
> others well-defined?
> 
> Jie Yu wrote:
> @Mpark,
> 
> I think the following check is in Resources::validate because otherwise 
> isReserved will break (e.g., role = `*` and reservation is not set, 
> isReserved(resource, `*`) will return true).
> 
> ```
> if (resource.role() == "*" && resource.has_reservation()) {
>   return Error(
>   "Invalid reservation: role \"*\" cannot be dynamically reserved");
> }
> ```
> 
> Michael Park wrote:
> @Jie,
> > e.g., role = * and reservation is not set, isReserved(resource, *) will 
> return true
> 
> If you meant `role = * and reservation _is_ set`, then yes.
> 
> I'm saying that exact reasoning is also why these checks should be in 
> `Resources::validate`, because otherwise `isRevocable` will break.
> e.g. `reservation is set and revocable is set`, `isRevocable` will return 
> true.
> 
> Niklas Nielsen wrote:
> Hey guys - did you reach a conclusion?

MPark; how can we get closure on this?


- Niklas


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


On June 21, 2015, noon, Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35715/
> ---
> 
> (Updated June 21, 2015, noon)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `mesos.proto`, it specifies the expected state of revocable resource:
> 
> ```
> // ... Note that if this is set, 'disk' or 'reservation' cannot be set.
> optional RevocableInfo revocable = 9;
> ```
>   
> This expectation should be validated in `Resources::validate(const Resource& 
> resoure)`
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521 
>   src/tests/resources_tests.cpp 9f96b14a6a4ce416d044934dd7ab4d28e4bc7332 
> 
> Diff: https://reviews.apache.org/r/35715/diff/
> 
> 
> Testing
> ---
> 
> Added `RevocableResourceTest.Validation` + `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35715: Added revocable resource state validation.

[Michael Park]

> On June 21, 2015, 6:47 p.m., Vinod Kone wrote:
> > src/common/resources.cpp, lines 479-487
> > 
> >
> > These checks are done in master's validation.cpp
> 
> Michael Park wrote:
> Ah sorry, I missed that.
> 
> This reminded me of the discussion Jie and I had for 
> [r32140](https://reviews.apache.org/r/32140/) regarding where validations 
> should live. I think this validation belongs here rather than in master 
> validation.
> What we concluded from the discussion was that `Resources::validate` 
> should perform necessary validation to satisfy the invariant of the 
> `Resource` object.
> This enables methods that operate on `Resource` (e.g. 
> `Resources::isRevocable`) to assume its validity.
> 
> My notes:
> > Synced with Jie on IRC regarding this topic. We agreed that 
> `Resources::validate` needs to capture the invariant of the `Resource` object 
> which means it needs to invalidate the `role == "*" && has_reservation()` 
> state. This invariant is required for all the predicates as well as functions 
> such as `reserved()` and `unreserved()` to have well-defined behavior.
> 
> Jie's note:
> > Discussed with Mpark offline. We agreed that rule for 
> Resources::validate is that it should only perform necessary validation to 
> make sure all methods in Resources are well hahaved, and the validation 
> around * and reservation info is necessary for 'reserved/unreserved' to work 
> properly. Thus dropping the issue around validation.
> 
> Michael Park wrote:
> I found Jie's comment regarding this: 
> https://reviews.apache.org/r/33865/#comment133597
> 
> @Jie: My thought here was that these checks are necessary to make 
> `isRevocable` well-defined. The same way the check for `"*" resource cannot 
> be dynamically reserved` is necessary to make `isDynamicallyReserved` and 
> others well-defined?
> 
> Jie Yu wrote:
> @Mpark,
> 
> I think the following check is in Resources::validate because otherwise 
> isReserved will break (e.g., role = `*` and reservation is not set, 
> isReserved(resource, `*`) will return true).
> 
> ```
> if (resource.role() == "*" && resource.has_reservation()) {
>   return Error(
>   "Invalid reservation: role \"*\" cannot be dynamically reserved");
> }
> ```
> 
> Michael Park wrote:
> @Jie,
> > e.g., role = * and reservation is not set, isReserved(resource, *) will 
> return true
> 
> If you meant `role = * and reservation _is_ set`, then yes.
> 
> I'm saying that exact reasoning is also why these checks should be in 
> `Resources::validate`, because otherwise `isRevocable` will break.
> e.g. `reservation is set and revocable is set`, `isRevocable` will return 
> true.
> 
> Niklas Nielsen wrote:
> Hey guys - did you reach a conclusion?
> 
> Niklas Nielsen wrote:
> MPark; how can we get closure on this?

I asked Jie to look at it a while ago but I think he's quite busy. I'll discard 
it for now.


- Michael


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


On June 21, 2015, 7 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35715/
> ---
> 
> (Updated June 21, 2015, 7 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `mesos.proto`, it specifies the expected state of revocable resource:
> 
> ```
> // ... Note that if this is set, 'disk' or 'reservation' cannot be set.
> optional RevocableInfo revocable = 9;
> ```
>   
> This expectation should be validated in `Resources::validate(const Resource& 
> resoure)`
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521 
>   src/tests/resources_tests.cpp 9f96b14a6a4ce416d044934dd7ab4d28e4bc7332 
> 
> Diff: https://reviews.apache.org/r/35715/diff/
> 
> 
> Testing
> ---
> 
> Added `RevocableResourceTest.Validation` + `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 34361: converted hard-coded strings to consts

[Niklas Nielsen]

> On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, lines 3031-3034
> > 
> >
> > Why bother with all this? Why not just have `"key1"`, `"value1"`, 
> > `"key2"`, `"value2"` inlined appropriately throughout these tests. Very 
> > straightforward to read.
> 
> Colin Williams wrote:
> I think the issue with the changes remaining is that the test depends on 
> the same value occurring in several places. By consolidating to a variable 
> it's no longer possible for these values to get out of sync.
> 
> Niklas Nielsen wrote:
> Colin: exactly
> 
> Ben: We have label tests three places; in the master, slave and in the 
> modules and they use the same "foo", "bar", "baz", "qux" key value pairs.
> The idea was to centralize them, so they don't get out of date and to 
> avoid code duplication.
> 
> Does that make sense?
> 
> Ben Mahler wrote:
> Well, then let's just keep it simple and get rid of these special strings 
> by just making the tests use "key1", "value1", "key2", "value2", etc.
> 
> I'm not following your code duplication point, this introduces more code 
> (now there are additional global constants, which we like to avoid if 
> unnecessary).
> 
> Also, each test is ideally independent, why does the master's test for 
> labels affect the slave's test for labels? Let's say I need a test with 5 
> labels, you want to have 5x2=10 global constants to address this?
> 
> Niklas Nielsen wrote:
> Try to see how testLabelKey and testLabelValue was used previously and 
> "foo", "bar", "qux" was used on others and I created this ticket to unify the 
> way we do this, along with seeing these key value pair being created in the 
> slave and master tests.
> 
> Dispite the current patch, I do think we can simply and unify the test 
> label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
> which key pairs are being tested. The comments in the test code need to carry 
> a bunch of context, to make sense out of the label transformations 
> (especially across the library border for the hooks tests).
> 
> This is all in spirit of reducing the tech debt we introduced. We are on 
> the same page on not introducing more lines/key pairs than before. Let us 
> iterate on this and get back to you.
> 
> Colin Williams wrote:
> Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
> point of unifying label names into a centralized variable that aren't at all 
> related.
> 
> Niklas: I was a little confused by the original task description, so I 
> thought a sample patch would be a good discussion point. I don't entirely 
> understand the tech debt you're trying to solve. For example, it seems like 
> you're asking to have some constants defined somewhere that are used by both 
> master_test and slave_test, but the the similarities between these two only 
> seem incidental. I'm concerned that creating something like 
> CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
> 
> Niklas Nielsen wrote:
> Okay - thinking about this; I am on board with "key1", "value1" etc.
> 
> Colin: the tech debt is that we have some inlined constants (like "foo", 
> "bar", etc) and some which are constants (which have to be kept in sync 
> between hooks modules and test body).
> The idea was to unify the way we name the key value pairs.
> 
> Do you want to update this ticket to reflect this, or come with a new 
> patch set which tackles streaming the two?
> 
> Niklas Nielsen wrote:
> Ping :)
> 
> Colin Williams wrote:
> Sorry, my day job's been really all-consuming the last few days. I was 
> planning to update the ticket.
> 
> Colin Williams wrote:
> Ticket updated

Colin; thanks! should we keep this patch then, or will you be providing a new 
one?


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> ---
> 
> (Updated June 8, 2015, 12:05 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2637
> https://issues.apache.org/jira/browse/MESOS-2637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> converted hard-coded strings to consts
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 3ffde6d 
>   src/tests/master_tests.cpp ba3858f 
>   src/tests/slave_tests.cpp acae497 
> 
> Diff: https://reviews.apache.org/r/34361/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colin Willi

Re: Review Request 34361: converted hard-coded strings to consts

[Colin Williams]

> On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, lines 3031-3034
> > 
> >
> > Why bother with all this? Why not just have `"key1"`, `"value1"`, 
> > `"key2"`, `"value2"` inlined appropriately throughout these tests. Very 
> > straightforward to read.
> 
> Colin Williams wrote:
> I think the issue with the changes remaining is that the test depends on 
> the same value occurring in several places. By consolidating to a variable 
> it's no longer possible for these values to get out of sync.
> 
> Niklas Nielsen wrote:
> Colin: exactly
> 
> Ben: We have label tests three places; in the master, slave and in the 
> modules and they use the same "foo", "bar", "baz", "qux" key value pairs.
> The idea was to centralize them, so they don't get out of date and to 
> avoid code duplication.
> 
> Does that make sense?
> 
> Ben Mahler wrote:
> Well, then let's just keep it simple and get rid of these special strings 
> by just making the tests use "key1", "value1", "key2", "value2", etc.
> 
> I'm not following your code duplication point, this introduces more code 
> (now there are additional global constants, which we like to avoid if 
> unnecessary).
> 
> Also, each test is ideally independent, why does the master's test for 
> labels affect the slave's test for labels? Let's say I need a test with 5 
> labels, you want to have 5x2=10 global constants to address this?
> 
> Niklas Nielsen wrote:
> Try to see how testLabelKey and testLabelValue was used previously and 
> "foo", "bar", "qux" was used on others and I created this ticket to unify the 
> way we do this, along with seeing these key value pair being created in the 
> slave and master tests.
> 
> Dispite the current patch, I do think we can simply and unify the test 
> label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
> which key pairs are being tested. The comments in the test code need to carry 
> a bunch of context, to make sense out of the label transformations 
> (especially across the library border for the hooks tests).
> 
> This is all in spirit of reducing the tech debt we introduced. We are on 
> the same page on not introducing more lines/key pairs than before. Let us 
> iterate on this and get back to you.
> 
> Colin Williams wrote:
> Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
> point of unifying label names into a centralized variable that aren't at all 
> related.
> 
> Niklas: I was a little confused by the original task description, so I 
> thought a sample patch would be a good discussion point. I don't entirely 
> understand the tech debt you're trying to solve. For example, it seems like 
> you're asking to have some constants defined somewhere that are used by both 
> master_test and slave_test, but the the similarities between these two only 
> seem incidental. I'm concerned that creating something like 
> CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
> 
> Niklas Nielsen wrote:
> Okay - thinking about this; I am on board with "key1", "value1" etc.
> 
> Colin: the tech debt is that we have some inlined constants (like "foo", 
> "bar", etc) and some which are constants (which have to be kept in sync 
> between hooks modules and test body).
> The idea was to unify the way we name the key value pairs.
> 
> Do you want to update this ticket to reflect this, or come with a new 
> patch set which tackles streaming the two?
> 
> Niklas Nielsen wrote:
> Ping :)
> 
> Colin Williams wrote:
> Sorry, my day job's been really all-consuming the last few days. I was 
> planning to update the ticket.
> 
> Colin Williams wrote:
> Ticket updated
> 
> Niklas Nielsen wrote:
> Colin; thanks! should we keep this patch then, or will you be providing a 
> new one?

I'm not planning on providing a new one.


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> ---
> 
> (Updated June 8, 2015, 7:05 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2637
> https://issues.apache.org/jira/browse/MESOS-2637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> converted hard-coded strings to consts
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 3ffde6d 
>   src/tests/master_tests.cpp ba3858f 
>   src/tests/slave_tests.cpp acae497 
> 
> Diff: https://reviews.apache.or

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36547]

All tests passed.

- Mesos ReviewBot


On July 16, 2015, 4:55 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 4:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36412: Adjusted the revocable cpu isolator test.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On July 11, 2015, 12:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36412/
> ---
> 
> (Updated July 11, 2015, 12:09 a.m.)
> 
> 
> Review request for mesos, Ian Downes and Vinod Kone.
> 
> 
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adjusted the revocable cpu isolator test.
> 
> 
> Diffs
> -
> 
>   src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 
> 
> Diff: https://reviews.apache.org/r/36412/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 36411: Used low cpu.shares for revocable containers.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On July 11, 2015, 12:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36411/
> ---
> 
> (Updated July 11, 2015, 12:09 a.m.)
> 
> 
> Review request for mesos, Ian Downes and Vinod Kone.
> 
> 
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used low cpu.shares for revocable containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/constants.hpp 
> e6df4a29e9af8076d6454740afa61fce04c3d06b 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 
> 
> Diff: https://reviews.apache.org/r/36411/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34137: Add support for container image provisioners.

[Timothy Chen]

> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 418
> > 
> >
> > Is it possible for rootfs to not exist when we are here? If not, there 
> > should be a CHECK and a comment on what guarantees its presence (the fact 
> > that there is a forked pid?).

the field rootfs is just a option, so I think that's why you can just pass that 
in.


> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > include/mesos/slave/isolator.hpp, lines 70-71
> > 
> >
> > We don't align arguments for constructors this way IIRC.
> > 
> > ExecutorRunState(arg1,
> >  arg2,
> >  );

Looks like the latest revision has the right format right?


> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1249
> > 
> >
> > why include the containerid? doesn't the caller of this method already 
> > know that?

The caller knows this, but since this shows up in the log it's easier to 
correlate the error with a container id.


> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1280
> > 
> >
> > ditto. why include the container id?

You're right I don't tihnk containerId here is needed.


> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 818
> > 
> >
> > where is the update to launchFlags to add 'rootfs' ? which review?

Looks like this field is already added in master.
commit 15bf9f67e3d9489e49b2176e1e4221a1a47fd0c9
Author: Ian Downes 
Date:   Thu Dec 18 15:46:15 2014 -0800

Support chrooting in MesosContainerizer launch helper.

Review: https://reviews.apache.org/r/31444/


- Timothy


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


On July 12, 2015, 4:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> ---
> 
> (Updated July 12, 2015, 4:47 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The MesosContainerizer can optionally provision a root filesystem for the 
> containers.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 
>   src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ac2387eabded61c897a5016655aae10cd1bca91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 
>   src/slave/containerizer/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 
>   src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 
>   src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
>   src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
>   src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 
>   src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 
>   src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 
> 
> Diff: https://reviews.apache.org/r/34137/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36412: Adjusted the revocable cpu isolator test.

[Vinod Kone]

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



src/tests/isolator_tests.cpp (line 417)


I would recommend to keep the IDLE check because that code still exists.


- Vinod Kone


On July 11, 2015, 12:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36412/
> ---
> 
> (Updated July 11, 2015, 12:09 a.m.)
> 
> 
> Review request for mesos, Ian Downes and Vinod Kone.
> 
> 
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adjusted the revocable cpu isolator test.
> 
> 
> Diffs
> -
> 
>   src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 
> 
> Diff: https://reviews.apache.org/r/36412/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 36413: Removed the code of setting SCHED_IDLE policy for revocable containers.

[Vinod Kone]

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

Ship it!



src/slave/containerizer/isolators/cgroups/cpushare.cpp 


I see. So you removed it here. Can you remove it from the test in this 
review instead of the previous one?


- Vinod Kone


On July 11, 2015, 12:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36413/
> ---
> 
> (Updated July 11, 2015, 12:09 a.m.)
> 
> 
> Review request for mesos, Ian Downes and Vinod Kone.
> 
> 
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed the code of setting SCHED_IDLE policy for revocable containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 
> 
> Diff: https://reviews.apache.org/r/36413/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34137: Add support for container image provisioners.

[Timothy Chen]

> On July 14, 2015, 7:41 p.m., Vinod Kone wrote:
> > src/slave/containerizer/provisioner.cpp, lines 43-56
> > 
> >
> > Why do you need foreach loop here if you were going to return error 
> > anyway?
> 
> Timothy Chen wrote:
> We need the foreach to go over all the provisioners though, as there 
> could be more than one although there is one listed for now.

I can remove the for loop and just return empty map, and also add a log that 
provisioners are not supported yet if something is specified.
Sounds good?


- Timothy


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


On July 12, 2015, 4:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> ---
> 
> (Updated July 12, 2015, 4:47 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The MesosContainerizer can optionally provision a root filesystem for the 
> containers.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 
>   src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ac2387eabded61c897a5016655aae10cd1bca91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 
>   src/slave/containerizer/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 
>   src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 
>   src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
>   src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
>   src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 
>   src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 
>   src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 
> 
> Diff: https://reviews.apache.org/r/34137/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Joerg Schad]

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

Ship it!



src/launcher/fetcher.cpp (line 134)


I assume those are spaces? Could you please doublecheck?



src/launcher/fetcher.cpp (line 136)


can you add a short comment why we assume this here?


- Joerg Schad


On July 16, 2015, 4:55 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 4:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.

[Vinod Kone]

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

Ship it!



src/tests/scheduler_event_call_tests.cpp (line 91)


Can you add a comment here saying that you are simulating master sending 
the event?


- Vinod Kone


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36494/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36494/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36495: Implemented the RESCIND Event handler in the scheduler driver.

[Vinod Kone]

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

Ship it!



src/sched/sched.cpp (line 469)


Add a TODO here to refactor.



src/tests/scheduler_event_call_tests.cpp (line 85)


ditto. comment.


- Vinod Kone


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36495/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36495/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.

[Vinod Kone]

> On July 15, 2015, 8:46 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, lines 490-493
> > 
> >
> > instead of calling into frameworkMessage() here can you have 
> > frameworkMessage() call into a new message(const Event& event) method?
> > 
> > this is how we did it in the master and i think it will make 
> > deprecating old message handlers easy.

if you want to add a TODO for now and follow up soon after, that would be good 
too.


- Vinod


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


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36494/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36494/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36496: Implemented the FAILURE Event handler in the scheduler driver.

[Vinod Kone]

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



src/sched/sched.cpp (line 509)


TODO for refactor.



src/tests/scheduler_event_call_tests.cpp (line 170)


comment.


- Vinod Kone


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36496/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36496/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36496: Implemented the FAILURE Event handler in the scheduler driver.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36496/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36496/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36537: Made TaskState.data available via state.json endpoint.

[Ben Mahler]

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


We don't store 'data' because there are frameworks which send a lot of data, 
and this can OOM the master per MESOS-1746. Are you aware of this?

- Ben Mahler


On July 16, 2015, 2:54 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36537/
> ---
> 
> (Updated July 16, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Timothy Chen.
> 
> 
> Bugs: MESOS-3061
> https://issues.apache.org/jira/browse/MESOS-3061
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This would allows us to expose the docker container IP (that is queried via
> docker-inspect and is part of TaskState.data) to Mesos-DNS via
> Master state.json endpoint.
> 
> Currently, Master doesn't store TaskState::data and so it's not made 
> available via state.json. A follow up patch would fix it.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 
> 
> Diff: https://reviews.apache.org/r/36537/diff/
> 
> 
> Testing
> ---
> 
> Tested by modifying the test_executor to send data with TASK_RUNNING status 
> update. The data showed up in Slave's state.json.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.

[Vinod Kone]

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



src/sched/sched.cpp (lines 453 - 454)


Do you want to add a CHECK to make sure that 'master' is also None? I 
propose that we get rid of 'master' altogether. See my comment below.



src/sched/sched.cpp (line 463)


except for the 3rd case. can you add that to the comment. you can copy 
paste what you wrote in the description of this review.



src/sched/sched.cpp (line 1393)


Why store both 'master' and 'masterInfo'? I think you can get rid of 
'master'. Gets us away from having to make sure they are in sync.



src/tests/scheduler_event_call_tests.cpp (line 59)


Can you expand on the comment here? This test is a bit complicated and 
could use some comments on what you are doing and testing.



src/tests/scheduler_event_call_tests.cpp (line 73)


Needs a comment on why you are dropping the message.



src/tests/scheduler_event_call_tests.cpp (line 76)


why pausing the clock? comment.



src/tests/scheduler_event_call_tests.cpp (lines 83 - 84)


Why are you doing this test?



src/tests/scheduler_event_call_tests.cpp (lines 89 - 91)


pull this below the expectation.



src/tests/scheduler_event_call_tests.cpp (line 97)


comment.



src/tests/scheduler_event_call_tests.cpp (line 101)


s/call/message/



src/tests/scheduler_event_call_tests.cpp (line 115)


comment.



src/tests/scheduler_event_call_tests.cpp (lines 119 - 121)


hmm. can you split this into its own test.

this test is huge!



src/tests/scheduler_event_call_tests.cpp (line 139)


comment.



src/tests/scheduler_event_call_tests.cpp (line 145)


ditto. split this into its own test.



src/tests/scheduler_event_call_tests.cpp (line 158)


comment.


- Vinod Kone


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36497/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This one is non-trivial, note that we follow MESOS-786 with the exception of 
> the 3rd case, since it is not possible and schedulers could not have possibly 
> relied on getting registered instead of re-registered in that case.
> 
> We now need to store the MasterInfo coming from the detector, as it is not 
> provided in Event.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36497/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36493: Added a stub Event message handler in the scheduler driver.

[Ben Mahler]

> On July 15, 2015, 7 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 437
> > 
> >
> > Do you know why it doesn't resolve? Does stringify() help?

It should from what I can tell since we are within mesos::internal and the 
operators are in mesos::, but it turns out that gcc at least isn't resolving 
them and is just printing 1, 2, etc. I noticed that this is also the case for 
our other example frameworks (e.g. no_executor_framework).


- Ben


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


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36493/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See MESOS-2910.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
> 
> Diff: https://reviews.apache.org/r/36493/diff/
> 
> 
> Testing
> ---
> 
> Each event is tested in the follow up patches.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35797: Updated Frameworkinfo.capabilities on framework re-registration to support adding capabilities

[Aditi Dixit]

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

(Updated July 16, 2015, 6:46 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description (updated)
---

This only updates the master, not the allocator. Added test too.


Diffs (updated)
-

  src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
  src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e 

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


Testing
---

make check


Thanks,

Aditi Dixit

Re: Review Request 36498: Implemented the OFFERS Event handler in the scheduler driver.

[Vinod Kone]

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

Ship it!



src/sched/sched.cpp (lines 494 - 495)


couldn't you have used ANY which is the default?



src/tests/scheduler_event_call_tests.cpp (line 176)


This test also ensures that framework to executor messages can bypass the 
master right? Mind adding that here?



src/tests/scheduler_event_call_tests.cpp (line 211)


s/offers/ResourceOfferMessages/



src/tests/scheduler_event_call_tests.cpp (lines 230 - 235)


use createTask()?



src/tests/scheduler_event_call_tests.cpp (line 237)


Why capture the executor driver when it's not used?


- Vinod Kone


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36498/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910 and MESOS-3012
> https://issues.apache.org/jira/browse/MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-3012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This relies on 'Offer.url' to compute the pids needed for the message passing 
> optimization (see 
> [MESOS-3012](https://issues.apache.org/jira/browse/MESOS-3012)).
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36498/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36499: Implemented the UPDATE Event handler in the scheduler driver.

[Vinod Kone]

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

Ship it!



src/sched/sched.cpp (line 547)


TODO refactor.



src/tests/scheduler_event_call_tests.cpp (lines 331 - 332)


why this check?



src/tests/scheduler_event_call_tests.cpp (line 380)


No need for driver.stop() and driver.join()? Shutdown()?


- Vinod Kone


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36499/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a 
>   src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36499/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35797: Updated Frameworkinfo.capabilities on framework re-registration to support adding capabilities

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35797]

All tests passed.

- Mesos ReviewBot


On July 16, 2015, 6:46 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35797/
> ---
> 
> (Updated July 16, 2015, 6:46 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2880
> https://issues.apache.org/jira/browse/MESOS-2880
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This only updates the master, not the allocator. Added test too.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/tests/fault_tolerance_tests.cpp 
> 1070ccf17f98f6b3800684a5edd6517d90631c3e 
> 
> Diff: https://reviews.apache.org/r/35797/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 36450: Introduced Address and URL protobufs.

[Ben Mahler]

> On July 15, 2015, 7:08 p.m., Vinod Kone wrote:
> > src/common/type_utils.cpp, line 131
> > 
> >
> > Is the order of query parameters important? Aren't these URLs 
> > equivalent?
> > 
> > http://a.b.c/?k1=a&k2=b
> > 
> > http://a.b.c/?k2=b&k1=a

Java's URI class considers ordering as important:

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29

I'd also like to keep it simple for now, you'll notice that they consider 
percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we can 
just avoid this for now:

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29

Ideally, we'd have a URI / URL class in stout, where we can have a 
comprehensive equality operator.


- Ben


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


On July 15, 2015, 1:01 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36450/
> ---
> 
> (Updated July 15, 2015, 1:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-3012
> https://issues.apache.org/jira/browse/MESOS-3012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make the API more consistent, we'd like to have a single way to express a 
> network address.
> Also would like a way to express an HTTP address (a URL), which may include a 
> path prefix.
> 
> This also enables the message passing optimization in the scheduler driver 
> when receiving events, per MESOS-3012.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
>   include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a 
>   src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf 
>   src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d 
>   src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 
> 
> Diff: https://reviews.apache.org/r/36450/diff/
> 
> 
> Testing
> ---
> 
> Modified the simplest test I could find for offers.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.

[Ben Mahler]

> On July 16, 2015, 6:20 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, line 91
> > 
> >
> > Can you add a comment here saying that you are simulating master 
> > sending the event?

Hm.. I'll have to scatter this comment across all the tests I've added in this 
chain, I'll add a comment at the 'SchedulerDriverEventTest' level instead since 
all the tests post events manually. Good?


- Ben


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


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36494/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36494/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36450: Introduced Address and URL protobufs.

[Anand Mazumdar]

> On July 15, 2015, 7:08 p.m., Vinod Kone wrote:
> > src/common/type_utils.cpp, line 131
> > 
> >
> > Is the order of query parameters important? Aren't these URLs 
> > equivalent?
> > 
> > http://a.b.c/?k1=a&k2=b
> > 
> > http://a.b.c/?k2=b&k1=a
> 
> Ben Mahler wrote:
> Java's URI class considers ordering as important:
> 
> 
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29
> 
> I'd also like to keep it simple for now, you'll notice that they consider 
> percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we 
> can just avoid this for now:
> 
> 
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29
> 
> Ideally, we'd have a URI / URL class in stout, where we can have a 
> comprehensive equality operator.

+1 

There is already a URL struct that exists in libprocess that we might consider 
enriching upon later and also move it to stout.


- Anand


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


On July 15, 2015, 1:01 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36450/
> ---
> 
> (Updated July 15, 2015, 1:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-3012
> https://issues.apache.org/jira/browse/MESOS-3012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make the API more consistent, we'd like to have a single way to express a 
> network address.
> Also would like a way to express an HTTP address (a URL), which may include a 
> path prefix.
> 
> This also enables the message passing optimization in the scheduler driver 
> when receiving events, per MESOS-3012.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
>   include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a 
>   src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf 
>   src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d 
>   src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 
> 
> Diff: https://reviews.apache.org/r/36450/diff/
> 
> 
> Testing
> ---
> 
> Modified the simplest test I could find for offers.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36537: Made TaskState.data available via state.json endpoint.

[Kapil Arya]

> On July 16, 2015, 2:38 p.m., Ben Mahler wrote:
> > We don't store 'data' because there are frameworks which send a lot of 
> > data, and this can OOM the master per MESOS-1746. Are you aware of this?

Yes. That's why I haven't created the patch yet :-). I am still trying to 
explore avenues that can allow us to expose the `docker inspect` output _only_. 
One somewhat hackish way is to detect and save `docker inspect` run. An 
alternative is to create TaskStatus labels that are exposed via state.json. The 
latter would also allow us to create label decorator hooks for Slave modules.


- Kapil


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


On July 16, 2015, 10:54 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36537/
> ---
> 
> (Updated July 16, 2015, 10:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Timothy Chen.
> 
> 
> Bugs: MESOS-3061
> https://issues.apache.org/jira/browse/MESOS-3061
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This would allows us to expose the docker container IP (that is queried via
> docker-inspect and is part of TaskState.data) to Mesos-DNS via
> Master state.json endpoint.
> 
> Currently, Master doesn't store TaskState::data and so it's not made 
> available via state.json. A follow up patch would fix it.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 
> 
> Diff: https://reviews.apache.org/r/36537/diff/
> 
> 
> Testing
> ---
> 
> Tested by modifying the test_executor to send data with TASK_RUNNING status 
> update. The data showed up in Slave's state.json.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 36450: Introduced Address and URL protobufs.

[Ben Mahler]

> On July 15, 2015, 7:08 p.m., Vinod Kone wrote:
> > src/common/type_utils.cpp, line 131
> > 
> >
> > Is the order of query parameters important? Aren't these URLs 
> > equivalent?
> > 
> > http://a.b.c/?k1=a&k2=b
> > 
> > http://a.b.c/?k2=b&k1=a
> 
> Ben Mahler wrote:
> Java's URI class considers ordering as important:
> 
> 
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29
> 
> I'd also like to keep it simple for now, you'll notice that they consider 
> percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we 
> can just avoid this for now:
> 
> 
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29
> 
> Ideally, we'd have a URI / URL class in stout, where we can have a 
> comprehensive equality operator.
> 
> Anand Mazumdar wrote:
> +1 
> 
> There is already a URL struct that exists in libprocess that we might 
> consider enriching upon later and also move it to stout.

I'll add a TODO related to this.


- Ben


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


On July 15, 2015, 1:01 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36450/
> ---
> 
> (Updated July 15, 2015, 1:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-3012
> https://issues.apache.org/jira/browse/MESOS-3012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make the API more consistent, we'd like to have a single way to express a 
> network address.
> Also would like a way to express an HTTP address (a URL), which may include a 
> path prefix.
> 
> This also enables the message passing optimization in the scheduler driver 
> when receiving events, per MESOS-3012.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
>   include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a 
>   src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf 
>   src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d 
>   src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 
> 
> Diff: https://reviews.apache.org/r/36450/diff/
> 
> 
> Testing
> ---
> 
> Modified the simplest test I could find for offers.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.

[Vinod Kone]

> On July 16, 2015, 6:20 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, line 91
> > 
> >
> > Can you add a comment here saying that you are simulating master 
> > sending the event?
> 
> Ben Mahler wrote:
> Hm.. I'll have to scatter this comment across all the tests I've added in 
> this chain, I'll add a comment at the 'SchedulerDriverEventTest' level 
> instead since all the tests post events manually. Good?

sg.


- Vinod


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


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36494/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36494/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36499: Implemented the UPDATE Event handler in the scheduler driver.

[Ben Mahler]

> On July 16, 2015, 6:57 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, lines 331-332
> > 
> >
> > why this check?

I need the frameworkId from the message, so wanted to make sure the message 
parsed correctly before grabbing the framework id.


> On July 16, 2015, 6:57 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, line 380
> > 
> >
> > No need for driver.stop() and driver.join()? Shutdown()?

The driver destructor and the mesos test cleanup look sufficient. Was there 
something about this test that made you ask? Note that I don't do explicit 
stopping, joining, shutdown in the other tests.


- Ben


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


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36499/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a 
>   src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36499/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36537: Made TaskState.data available via state.json endpoint.

[Ben Mahler]

> On July 16, 2015, 6:38 p.m., Ben Mahler wrote:
> > We don't store 'data' because there are frameworks which send a lot of 
> > data, and this can OOM the master per MESOS-1746. Are you aware of this?
> 
> Kapil Arya wrote:
> Yes. That's why I haven't created the patch yet :-). I am still trying to 
> explore avenues that can allow us to expose the `docker inspect` output 
> _only_. One somewhat hackish way is to detect and save `docker inspect` run. 
> An alternative is to create TaskStatus labels that are exposed via 
> state.json. The latter would also allow us to create label decorator hooks 
> for Slave modules.

Isn't sending docker inspect already a hack?

What if folks have a custom executor in their docker container? Then they won't 
be getting 'docker inspect' from the docker/executor.cpp, right?


- Ben


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


On July 16, 2015, 2:54 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36537/
> ---
> 
> (Updated July 16, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Timothy Chen.
> 
> 
> Bugs: MESOS-3061
> https://issues.apache.org/jira/browse/MESOS-3061
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This would allows us to expose the docker container IP (that is queried via
> docker-inspect and is part of TaskState.data) to Mesos-DNS via
> Master state.json endpoint.
> 
> Currently, Master doesn't store TaskState::data and so it's not made 
> available via state.json. A follow up patch would fix it.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 
> 
> Diff: https://reviews.apache.org/r/36537/diff/
> 
> 
> Testing
> ---
> 
> Tested by modifying the test_executor to send data with TASK_RUNNING status 
> update. The data showed up in Slave's state.json.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 35797: Updated Frameworkinfo.capabilities on framework re-registration to support adding capabilities

[Vinod Kone]

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



src/master/master.hpp (line 534)


s/capabilities_size()/capabilities.size()/



src/master/master.hpp (line 536)


Do you also want to deal with the case where capabilities are previously 
set but now being unset? In other words, the "else" case.

if (source.capabilities.size() > 0 ) {
  info.mutable_capabilities()->CopyFrom(source.capabilities);
} else {
  info.clear_capabilities();
}

If you want do this in a follow up patch, add a TODO here. You will also 
need a new test for this.



src/tests/fault_tolerance_tests.cpp (line 1730)


Add a comment on the top of the test that you are verifying that when a 
framework re-registers with updated framework info, it gets updated in the 
master.



src/tests/fault_tolerance_tests.cpp (lines 1742 - 1743)


We now require a much newer gcc version, so this shouldn't be an issue. You 
can just do.

FrameworkInfo framework1 = DEFAULT_FRAMEWORK_INFO;



src/tests/fault_tolerance_tests.cpp (lines 1769 - 1770)


ditto. just assign on the same line.



src/tests/fault_tolerance_tests.cpp (line 1772)


s/Name/Type/



src/tests/fault_tolerance_tests.cpp (lines 1773 - 1774)


Can you also update webui_url, hostname and failover_timeout while you are 
at it?



src/tests/fault_tolerance_tests.cpp (line 1803)


s/masterState/response/



src/tests/fault_tolerance_tests.cpp (line 1806)


s/masterStateObject/parse/

you also need a

ASSERT_SOME(parse);



src/tests/fault_tolerance_tests.cpp (line 1809)


new line after ASSERT.

s/masterStateObject_/state/



src/tests/fault_tolerance_tests.cpp (line 1812)


s/famework_/framework/

new line after this.



src/tests/fault_tolerance_tests.cpp (line 1831)


Nice test!


- Vinod Kone


On July 16, 2015, 6:46 p.m., Aditi Dixit wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35797/
> ---
> 
> (Updated July 16, 2015, 6:46 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2880
> https://issues.apache.org/jira/browse/MESOS-2880
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This only updates the master, not the allocator. Added test too.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/tests/fault_tolerance_tests.cpp 
> 1070ccf17f98f6b3800684a5edd6517d90631c3e 
> 
> Diff: https://reviews.apache.org/r/35797/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Aditi Dixit
> 
>

Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.

[Paul Brett]

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

(Updated July 16, 2015, 9:32 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.


Changes
---

Removed the dependency on 36424


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


Repository: mesos


Description
---

Refactor Linux Performance monitor to handle changing 'perf stat' output 
versions depending on kernel version.


Diffs (updated)
-

  src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 36498: Implemented the OFFERS Event handler in the scheduler driver.

[Ben Mahler]

> On July 16, 2015, 6:52 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, lines 494-495
> > 
> >
> > couldn't you have used ANY which is the default?

I only want to remove from the front and the end, ANY will remove from anywhere 
in the string.

I'll use strings::trim instead, where ANY means front and back as you expected 
(although ANY is a bit confusing in this context :)).


- Ben


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


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36498/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910 and MESOS-3012
> https://issues.apache.org/jira/browse/MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-3012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This relies on 'Offer.url' to compute the pids needed for the message passing 
> optimization (see 
> [MESOS-3012](https://issues.apache.org/jira/browse/MESOS-3012)).
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36498/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Timothy Chen]

> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > So I found the use of the field `id` inconsistent in the code.
> > 
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> > 
> > I think we should consistently refer to `id` using the definition in 
> > the 
> > [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id),
> >  i.e., with the prefix.
> > 
> > The fact that the ID is computed by the image creator using sha512 and 
> > that the provisioner validates it using sha512 is merely an implementation 
> > detail that is not a conern of higher level abstractions / APIs.
> > 
> > So here I think in the comments we should not call it "Image hash" but 
> > rather refer to the spec for its full definition. We can of course call out 
> > the fact that it should have the "sha512-" perfix.
> > 
> > What do you think?

Hi there,
I'm going to commit this for Ian and just saw your comment.
How about I reword the comment here to "// The ID of the Image. Please refer to 
the Appc spec for its definition."?


- Timothy


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


On July 12, 2015, 4:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 12, 2015, 4:47 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 36560: Made Subscribe.force optional in the Call protobuf.

[Vinod Kone]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

Made 'force' optional because it doesn't make sense when subscribing without an 
id.


Diffs
-

  include/mesos/scheduler/scheduler.proto 
a027da255563c620fa3d7355ad47aa16d2264f77 
  src/examples/event_call_framework.cpp 
17fdcac44c0a51293a318ef5184f4d48a461abd9 
  src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.

[Vinod Kone]

> On July 15, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/tests/scheduler_tests.cpp, line 143
> > 
> >
> > Hm.. 'force' doesn't make sense for SUBSCRIBE without a framework id.
> > 
> > Seems like either we:
> > 
> > (1) Make 'force' optional, and document that it is only relevant when 
> > the framework id is set (re-subscription).
> > 
> > (2) Remove 'force' from SUBSCRIBE, add a RESUBSCRIBE with 'force'? =/

I'll make 'force' optional in a dependent different review.


> On July 15, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/tests/scheduler_tests.cpp, line 163
> > 
> >
> > why the newline here but not above?

looked dense. will remove.


> On July 15, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/tests/scheduler_tests.cpp, lines 110-112
> > 
> >
> > Since there's no RESUBSCRIBE, shall we simply call this test 
> > 'Subscribe' (I noticed there is no Subscribe test currently) and test the 
> > full semantics within it? Looks like a test of the 'Subscribe' call to me.

done.


> On July 15, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1747
> > 
> >
> > Seems like we might want to keep the condition consistent across all of 
> > these checks (i.e. has_id && id != ""), up to you.
> > 
> > At least, would be nice to add an != operator for FrameworkID vs string.

updated the condition. i'll add a TODO to add != operator and do a sweep.


- Vinod


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


On July 15, 2015, 6:40 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36518/
> ---
> 
> (Updated July 15, 2015, 6:40 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3055
> https://issues.apache.org/jira/browse/MESOS-3055
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the process of fixing this, added an additional check in 
> Master::registerFramework() that should've been there in the first place. 
> Similar check exists in Master::reregisterFramework().
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d 
>   src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 
> 
> Diff: https://reviews.apache.org/r/36518/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.

[Vinod Kone]

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

(Updated July 16, 2015, 9:48 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's comments. NNFR.


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


Repository: mesos


Description
---

In the process of fixing this, added an additional check in 
Master::registerFramework() that should've been there in the first place. 
Similar check exists in Master::reregisterFramework().


Diffs (updated)
-

  src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d 
  src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 34136: Add ContainerImage protobuf.

[Timothy Chen]

> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > So I found the use of the field `id` inconsistent in the code.
> > 
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> > 
> > I think we should consistently refer to `id` using the definition in 
> > the 
> > [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id),
> >  i.e., with the prefix.
> > 
> > The fact that the ID is computed by the image creator using sha512 and 
> > that the provisioner validates it using sha512 is merely an implementation 
> > detail that is not a conern of higher level abstractions / APIs.
> > 
> > So here I think in the comments we should not call it "Image hash" but 
> > rather refer to the spec for its full definition. We can of course call out 
> > the fact that it should have the "sha512-" perfix.
> > 
> > What do you think?
> 
> Timothy Chen wrote:
> Hi there,
> I'm going to commit this for Ian and just saw your comment.
> How about I reword the comment here to "// The ID of the Image. Please 
> refer to the Appc spec for its definition."?

Actually I mis-read what you meant, how about:

 // The ID of the Image.
  // An image ID is canonically represented as a string prefixed by
  // the algorithm used and the hash output (e.g. sha512-a83...).


- Timothy


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


On July 12, 2015, 4:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 12, 2015, 4:47 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36560: Made Subscribe.force optional in the Call protobuf.

[Ben Mahler]

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

Ship it!



include/mesos/scheduler/scheduler.proto (lines 175 - 186)


Hm.. this seems more confusing, why not just say in the beginning that this 
is only relevant for frameworks that have been assigned an ID?

e.g.

// If frameowrk_info.id is present, 'force' tells the master ...


- Ben Mahler


On July 16, 2015, 9:48 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36560/
> ---
> 
> (Updated July 16, 2015, 9:48 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3055
> https://issues.apache.org/jira/browse/MESOS-3055
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made 'force' optional because it doesn't make sense when subscribing without 
> an id.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> a027da255563c620fa3d7355ad47aa16d2264f77 
>   src/examples/event_call_framework.cpp 
> 17fdcac44c0a51293a318ef5184f4d48a461abd9 
>   src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 
> 
> Diff: https://reviews.apache.org/r/36560/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Jiang Yan Xu]

> On July 14, 2015, 2:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > So I found the use of the field `id` inconsistent in the code.
> > 
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> > 
> > I think we should consistently refer to `id` using the definition in 
> > the 
> > [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id),
> >  i.e., with the prefix.
> > 
> > The fact that the ID is computed by the image creator using sha512 and 
> > that the provisioner validates it using sha512 is merely an implementation 
> > detail that is not a conern of higher level abstractions / APIs.
> > 
> > So here I think in the comments we should not call it "Image hash" but 
> > rather refer to the spec for its full definition. We can of course call out 
> > the fact that it should have the "sha512-" perfix.
> > 
> > What do you think?
> 
> Timothy Chen wrote:
> Hi there,
> I'm going to commit this for Ian and just saw your comment.
> How about I reword the comment here to "// The ID of the Image. Please 
> refer to the Appc spec for its definition."?
> 
> Timothy Chen wrote:
> Actually I mis-read what you meant, how about:
> 
>  // The ID of the Image.
>   // An image ID is canonically represented as a string prefixed by
>   // the algorithm used and the hash output (e.g. sha512-a83...).

Or we could just copy the definition here verbatim. :)

https://github.com/appc/spec/blob/master/spec/types.md#image-id-type

An image ID is a string of the format "hash-value", where "hash" is the hash 
algorithm used and "value" is the hex encoded string of the digest. Currently 
the only permitted hash algorithm is sha512.

Thanks, please commit it!


- Jiang Yan


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


On July 11, 2015, 9:47 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 11, 2015, 9:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Timothy Chen]

> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > So I found the use of the field `id` inconsistent in the code.
> > 
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> > 
> > I think we should consistently refer to `id` using the definition in 
> > the 
> > [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id),
> >  i.e., with the prefix.
> > 
> > The fact that the ID is computed by the image creator using sha512 and 
> > that the provisioner validates it using sha512 is merely an implementation 
> > detail that is not a conern of higher level abstractions / APIs.
> > 
> > So here I think in the comments we should not call it "Image hash" but 
> > rather refer to the spec for its full definition. We can of course call out 
> > the fact that it should have the "sha512-" perfix.
> > 
> > What do you think?
> 
> Timothy Chen wrote:
> Hi there,
> I'm going to commit this for Ian and just saw your comment.
> How about I reword the comment here to "// The ID of the Image. Please 
> refer to the Appc spec for its definition."?
> 
> Timothy Chen wrote:
> Actually I mis-read what you meant, how about:
> 
>  // The ID of the Image.
>   // An image ID is canonically represented as a string prefixed by
>   // the algorithm used and the hash output (e.g. sha512-a83...).
> 
> Jiang Yan Xu wrote:
> Or we could just copy the definition here verbatim. :)
> 
> https://github.com/appc/spec/blob/master/spec/types.md#image-id-type
> 
> An image ID is a string of the format "hash-value", where "hash" is the 
> hash algorithm used and "value" is the hex encoded string of the digest. 
> Currently the only permitted hash algorithm is sha512.
> 
> Thanks, please commit it!

Thanks!


- Timothy


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


On July 12, 2015, 4:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 12, 2015, 4:47 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36560, 36518]

All tests passed.

- Mesos ReviewBot


On July 16, 2015, 9:48 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36518/
> ---
> 
> (Updated July 16, 2015, 9:48 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3055
> https://issues.apache.org/jira/browse/MESOS-3055
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the process of fixing this, added an additional check in 
> Master::registerFramework() that should've been there in the first place. 
> Similar check exists in Master::reregisterFramework().
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d 
>   src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 
> 
> Diff: https://reviews.apache.org/r/36518/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36488: Added oversubscription user doc.

[Niklas Nielsen]

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

(Updated July 16, 2015, 3:39 p.m.)


Review request for mesos, Adam B and Jie Yu.


Repository: mesos


Description
---

Added first draft of oversubscription user doc


Diffs (updated)
-

  docs/images/oversubscription-overview.jpg PRE-CREATION 
  docs/oversubscription.md PRE-CREATION 

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


Testing
---

Rendered at: 
https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md


Thanks,

Niklas Nielsen

Re: Review Request 36488: Added oversubscription user doc.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [36488]

Failed command: ./support/apply-review.sh -n -r 36488

Error:
 2015-07-16 22:52:15 URL:https://reviews.apache.org/r/36488/diff/raw/ 
[10352/10352] -> "36488.patch" [1]
error: missing binary patch data for 'docs/images/oversubscription-overview.jpg'
error: binary patch does not apply to 
'docs/images/oversubscription-overview.jpg'
error: docs/images/oversubscription-overview.jpg: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On July 16, 2015, 10:39 p.m., Niklas Nielsen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36488/
> ---
> 
> (Updated July 16, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added first draft of oversubscription user doc
> 
> 
> Diffs
> -
> 
>   docs/images/oversubscription-overview.jpg PRE-CREATION 
>   docs/oversubscription.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36488/diff/
> 
> 
> Testing
> ---
> 
> Rendered at: 
> https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Re: Review Request 34137: Add support for container image provisioners.

[Timothy Chen]

> On May 28, 2015, 9:49 p.m., Paul Brett wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, line 245
> > 
> >
> > To many underscores - can we come up with a better name?

We can refactor later.


- Timothy


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


On July 12, 2015, 4:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> ---
> 
> (Updated July 12, 2015, 4:47 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The MesosContainerizer can optionally provision a root filesystem for the 
> containers.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 
>   src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ac2387eabded61c897a5016655aae10cd1bca91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 
>   src/slave/containerizer/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 
>   src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 
>   src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
>   src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
>   src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 
>   src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 
>   src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 
> 
> Diff: https://reviews.apache.org/r/34137/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36488: Added oversubscription user doc.

[Jie Yu]

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


Looks good Nik!


docs/oversubscription.md (line 6)


for majority of the time



docs/oversubscription.md (line 8)


we can oversubscribe unallocated resources as well. So probably say

```
Oversubscription takes advantage of temporarily unused resources to ...
```



docs/oversubscription.md (line 28)


s/resource slack/resource usage slack/

and also allocation slack?



docs/oversubscription.md (lines 30 - 31)


I thought we use a pull model eventually. So maybe:

```
The slave keeps polling estimates from the resource estimator and tracks 
the latest estimate.
```



docs/oversubscription.md (lines 33 - 34)


The slave only send the message to the master if there's a change to the 
previous estimate.



docs/oversubscription.md (line 39)


The resource is revocable (not the offer).

```
and marks those resources as `revocable`.
```



docs/oversubscription.md (line 40)


up to the resource estimator to determine which types...



docs/oversubscription.md (line 46)


Again, it's the resource that's revocable, not the offer.



docs/oversubscription.md (line 49)


revocable resources.



docs/oversubscription.md (lines 55 - 58)


I think it's worthwhile to mention that if any resource used by a 
task/executor is revocable, the whole container is treated as a revocable 
container (meaning can be killed or throttled).



docs/oversubscription.md (line 69)


Frameworks planning to use oversubscribed resources need to register ...



docs/oversubscription.md (line 80)


I would say:

```
, the framework will start to receive revocable resources in offers.
```



docs/oversubscription.md (line 84)


See below for instructions how to configure Mesos for oversubscription.



docs/oversubscription.md (line 86)


Launching tasks using revocable resources.



docs/oversubscription.md (line 88)


This is not true currently. Revocable resources and regular resources will 
be sent in the same offer.



docs/oversubscription.md (line 95)


Again, currently, we don't split offers. So you may want to use an example 
that has one offer but mixed type of resources.



docs/oversubscription.md (line 148)


with fixed amount of resources.



docs/oversubscription.md (line 288)


Maybe you want to also document how to configure the fixed resource 
estimator? It's configured through modules, but shipped with Mesos?

```
--resource_estimator="org_apache_mesos_FixedResourceEstimator"

--modules="libraries {
  file: "/usr/local/lib64/libfixed_resource_estimator.so"
  modules {
name: "org_apache_mesos_FixedResourceEstimator"
parameters {
  key: "resources"
  value: "cpus:14"
}
  }
}

--
```


- Jie Yu


On July 16, 2015, 10:39 p.m., Niklas Nielsen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36488/
> ---
> 
> (Updated July 16, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added first draft of oversubscription user doc
> 
> 
> Diffs
> -
> 
>   docs/images/oversubscription-overview.jpg PRE-CREATION 
>   docs/oversubscription.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36488/diff/
> 
> 
> Testing
> ---
> 
> Rendered at: 
> https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.

[Ben Mahler]

> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> >

Split the test apart.


> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 1393
> > 
> >
> > Why store both 'master' and 'masterInfo'? I think you can get rid of 
> > 'master'. Gets us away from having to make sure they are in sync.

Done, pulled this out into a separate review.


> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, line 59
> > 
> >
> > Can you expand on the comment here? This test is a bit complicated and 
> > could use some comments on what you are doing and testing.

Split the test per your other suggestion.


> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, lines 83-84
> > 
> >
> > Why are you doing this test?

To get the framework id from the message, is there a better way?


> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, lines 89-91
> > 
> >
> > pull this below the expectation.

Hm.. will require a sweep across all the tests here, so I'll punt since it's 
not posssible to do consistently in the tests (some tests require things from 
the event in the expectation). Ok?


> On July 16, 2015, 6:41 p.m., Vinod Kone wrote:
> > src/tests/scheduler_event_call_tests.cpp, line 101
> > 
> >
> > s/call/message/

It's the call that I'm testing, not the message :)


- Ben


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


On July 15, 2015, 1:47 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36497/
> ---
> 
> (Updated July 15, 2015, 1:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2910
> https://issues.apache.org/jira/browse/MESOS-2910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This one is non-trivial, note that we follow MESOS-786 with the exception of 
> the 3rd case, since it is not possible and schedulers could not have possibly 
> relied on getting registered instead of re-registered in that case.
> 
> We now need to store the MasterInfo coming from the detector, as it is not 
> provided in Event.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
>   src/tests/scheduler_event_call_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36497/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36461: Added FutureMessageType, DropMessagesType and ExpectNoFutureMessagesType.

[Vinod Kone]

> On July 15, 2015, 5:29 p.m., Ben Mahler wrote:
> > It looks like this could benefit from a bit of documentation that mentions 
> > the protobuf ["union 
> > technique"](https://developers.google.com/protocol-buffers/docs/techniques#union).

added a comment on top of the matcher.


> On July 15, 2015, 5:29 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/gmock.hpp, line 319
> > 
> >
> > Anything preventing s/t/type/ ?
> > 
> > Type in this context is a bit confusing since it sounds like the 
> > message type.
> > 
> > Can we call this something like 'UnionMessageMatcher' and avoid using 
> > the overloaded word "type"?

s/MessageTypeMatcher/UnionMessageMatcher/
s/t/unionType/


> On July 15, 2015, 5:29 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/gmock.hpp, line 322
> > 
> >
> > Anything preventing s/n/message/?

s/n/message/


- Vinod


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


On July 13, 2015, 11:55 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36461/
> ---
> 
> (Updated July 13, 2015, 11:55 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Needed these abstractions for capturing specific CALLs explicitly in 
> subsequent reviews.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp 
> 0fd3f8cf06c9efd357fa7c933cc28d527855ac9a 
> 
> Diff: https://reviews.apache.org/r/36461/diff/
> 
> 
> Testing
> ---
> 
> Tested in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36461: Added FutureMessageType, DropMessagesType and ExpectNoFutureMessagesType.

[Vinod Kone]

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

(Updated July 17, 2015, 12:48 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's comments.


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


Repository: mesos


Description
---

Needed these abstractions for capturing specific CALLs explicitly in subsequent 
reviews.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp 
e8781610f636954b39611fcb1de310a78ceea7cb 

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


Testing
---

Tested in subsequent reviews.


Thanks,

Vinod Kone

Re: Review Request 36462: Added FUTURE_CALL, DROP_CALL, DROP_CALLS and EXPECT_NO_FUTURE_CALLS.

[Vinod Kone]

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

(Updated July 17, 2015, 12:49 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's comments.


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


Repository: mesos


Description
---

Needed these abstractions for capturing specific CALLs explicitly in subsequent 
reviews.


Diffs (updated)
-

  src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 

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


Testing
---

Tested in subsequent reviews.


Thanks,

Vinod Kone

Re: Review Request 36462: Added FUTURE_CALL, DROP_CALL, DROP_CALLS and EXPECT_NO_FUTURE_CALLS.

[Ben Mahler]

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

Ship it!


Ship It!

- Ben Mahler


On July 17, 2015, 12:49 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36462/
> ---
> 
> (Updated July 17, 2015, 12:49 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Needed these abstractions for capturing specific CALLs explicitly in 
> subsequent reviews.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 
> 
> Diff: https://reviews.apache.org/r/36462/diff/
> 
> 
> Testing
> ---
> 
> Tested in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36464: Updated scheduler driver to send ACCEPT call.

[Vinod Kone]

> On July 15, 2015, 5:58 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, lines 1037-1050
> > 
> >
> > Mind pulling this patch out into a separate review? Seems independent :)

i'll just committ this as a standalone patch. ok with you?


- Vinod


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


On July 13, 2015, 11:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36464/
> ---
> 
> (Updated July 13, 2015, 11:59 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 
> 
> Diff: https://reviews.apache.org/r/36464/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36465: Updated scheduler driver to send REVIVE call.

[Vinod Kone]

> On July 15, 2015, 5:58 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, line 1029
> > 
> >
> > newline before setting the type, like your other reviews?

i'm doing new line after setting type. i'll make sure it's consistent.


- Vinod


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


On July 14, 2015, midnight, Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36465/
> ---
> 
> (Updated July 14, 2015, midnight)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
> 
> Diff: https://reviews.apache.org/r/36465/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> (NOTE: There is already an existing test that tests the revive workflow, but 
> it didn't need any update)
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36461: Added FutureMessageType, DropMessagesType and ExpectNoFutureMessagesType.

[Ben Mahler]

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

Ship it!


Ship It!

- Ben Mahler


On July 17, 2015, 12:48 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36461/
> ---
> 
> (Updated July 17, 2015, 12:48 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Needed these abstractions for capturing specific CALLs explicitly in 
> subsequent reviews.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp 
> e8781610f636954b39611fcb1de310a78ceea7cb 
> 
> Diff: https://reviews.apache.org/r/36461/diff/
> 
> 
> Testing
> ---
> 
> Tested in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36464: Updated scheduler driver to send ACCEPT call.

[Ben Mahler]

> On July 15, 2015, 5:58 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, lines 1037-1050
> > 
> >
> > Mind pulling this patch out into a separate review? Seems independent :)
> 
> Vinod Kone wrote:
> i'll just committ this as a standalone patch. ok with you?

yep


- Ben


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


On July 13, 2015, 11:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36464/
> ---
> 
> (Updated July 13, 2015, 11:59 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 
> 
> Diff: https://reviews.apache.org/r/36464/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 36562: Store MasterInfo instead of UPID in the scheduler driver.

[Ben Mahler]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

See https://reviews.apache.org/r/36497/ for context.


Diffs
-

  src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 

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


Testing
---

make check


Thanks,

Ben Mahler

Re: Review Request 34137: Add support for container image provisioners.

[Jie Yu]

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


Tim, I probably wound wait for Vinod's shipit before committing this.

There seems to be a bug in the code as well (besides the obvious broken build 
issue).


src/slave/containerizer/mesos/containerizer.cpp (line 629)


Hum, looks like a bug since, for example, slaveId is a reference and will 
be invalid when the lambda is called. In general, I think we should avoid using 
[=] for lambdas because its dangeous!

I would prefer we resort to our old style defer style (e.g., introduce 
`_provision`).



src/slave/containerizer/mesos/containerizer.cpp (lines 636 - 642)


No test for checkpointing???



src/slave/containerizer/mesos/containerizer.cpp (line 1236)


`_destroy` is a void function. We should do:

```
_destroy(...);
return;
```



src/slave/containerizer/mesos/containerizer.cpp (line 1247)


Ditto.



src/slave/containerizer/mesos/containerizer.cpp (line 1268)


s/cleanup/future/



src/slave/containerizer/provisioner.hpp (lines 37 - 38)


This should be #include 



src/slave/containerizer/provisioner.hpp (lines 52 - 54)


Recover containers? What container? This is a little misleading. How about

```
Recover root filesystems for containers...




src/slave/containerizer/provisioner.cpp (line 24)


Where is this? This won't compile!


- Jie Yu


On July 12, 2015, 4:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> ---
> 
> (Updated July 12, 2015, 4:47 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The MesosContainerizer can optionally provision a root filesystem for the 
> containers.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 
>   src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ac2387eabded61c897a5016655aae10cd1bca91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 
>   src/slave/containerizer/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 
>   src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 
>   src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
>   src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
>   src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 
>   src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 
>   src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 
> 
> Diff: https://reviews.apache.org/r/34137/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.

[Ben Mahler]

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

(Updated July 17, 2015, 1:36 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Split apart the test.


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


Repository: mesos


Description
---

This one is non-trivial, note that we follow MESOS-786 with the exception of 
the 3rd case, since it is not possible and schedulers could not have possibly 
relied on getting registered instead of re-registered in that case.

We now need to store the MasterInfo coming from the detector, as it is not 
provided in Event.


Diffs (updated)
-

  src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 
  src/tests/scheduler_event_call_tests.cpp 
cf6aa19a644580ff9d805e919763e9342d72677f 

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


Testing
---

Added a test.


Thanks,

Ben Mahler

Re: Review Request 36498: Implemented the OFFERS Event handler in the scheduler driver.

[Ben Mahler]

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

(Updated July 17, 2015, 1:36 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Cleanups per Vinod's review.


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


Repository: mesos


Description
---

This relies on 'Offer.url' to compute the pids needed for the message passing 
optimization (see 
[MESOS-3012](https://issues.apache.org/jira/browse/MESOS-3012)).


Diffs (updated)
-

  src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 
  src/tests/scheduler_event_call_tests.cpp 
cf6aa19a644580ff9d805e919763e9342d72677f 

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


Testing
---

Added a test.


Thanks,

Ben Mahler

Re: Review Request 36450: Introduced Address and URL protobufs.

[Ben Mahler]

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

(Updated July 17, 2015, 1:36 a.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Added a TODO to use process::URL equality.


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


Repository: mesos


Description
---

To make the API more consistent, we'd like to have a single way to express a 
network address.
Also would like a way to express an HTTP address (a URL), which may include a 
path prefix.

This also enables the message passing optimization in the scheduler driver when 
receiving events, per MESOS-3012.


Diffs (updated)
-

  include/mesos/mesos.proto d2f482668e671b30f2586f6aae9c20132ab4d1e4 
  include/mesos/type_utils.hpp eb7fe2562cfcff52288d1c216425068d1ba551c0 
  src/common/type_utils.cpp 2ad5b4cbe324c83e81fd7df7430652f5c0a4e30f 
  src/master/master.cpp 082758ef54597ad32cf0d025c147f0f44dd11961 
  src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc 

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


Testing
---

Modified the simplest test I could find for offers.


Thanks,

Ben Mahler

Re: Review Request 36463: Updated scheduler driver to send Kill Call.

[Vinod Kone]

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

(Updated July 17, 2015, 2:26 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's. NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f 
  src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e 
  src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36463: Updated scheduler driver to send Kill Call.

[Vinod Kone]

> On July 15, 2015, 5:58 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1359-1360
> > 
> >
> > Ditto here, do we need the mesos:: prefix?

yea, unfortunately it's needed, because otherwise the compiler gets confused as 
it looks in the mesos::internal::scheduler namespace


- Vinod


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


On July 13, 2015, 11:58 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36463/
> ---
> 
> (Updated July 13, 2015, 11:58 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/tests/fault_tolerance_tests.cpp 
> 1070ccf17f98f6b3800684a5edd6517d90631c3e 
>   src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 
> 
> Diff: https://reviews.apache.org/r/36463/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 34128: Enable different IP/Port for external access.

[Anindya Sinha]

> On June 11, 2015, 7:34 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 820-836
> > 
> >
> > If two libprocess based unix processes (e.g., scheudler and master) are 
> > within the *same* bridged container, would they able to communicate with 
> > this change? Can you test this to confirm?
> > 
> > 
> > If not, a better option might be to instead have LIBPROCESS_BIND_IP and 
> > LIBPROCESS_BIND_PORT that just changes the address we bind to. 
> > LIBPROCESS_IP and LIBPROCESS_PORT semantics could be left untouched.
> 
> Anindya Sinha wrote:
> If 2 libprocess based unix processes are running, they would point to a 
> different  (most likely same public_ip but a different 
> public_port, ie. same LIBPROCESS_PUBLIC_IP but a different 
> LIBPROCESS_PUBLIC_PORT). The processes themselves would bind as it does today 
> on  (based in LIBPROCESS_IP and LIBPROCESS_PORT). Once a request 
> lands on a corresponding , a proxy listening on that 
> would forward that to the actual  corresponding to the 
> .
> 
> As an example, mesos-master binds on 10.11.12.13:5050 (ip:port) with 
> public_ip:public_port as 192.168.100.100:6050, and say scheduler binds on 
> 10.11.12.13:8081 with public_ip:public_port as 192.168.100.100:9081. Requests 
> received on 192.168.100.100:6050 shall be proxied over to 10.11.12.13:5050 
> (to reach mesos-master) and requests received on 192.168.100.100:9081 shall 
> be proxied over to 10.11.12.13:8081 (to reach scheduler).
> 
> Vinod Kone wrote:
> ```
> a proxy listening on that would forward that...
> ```
> 
> who sets up this proxy? or do you mean this is what happens currently in 
> bridged mode containers (e.g., docker)?
> 
> Anindya Sinha wrote:
> No this is not something that is part of docker. The proxy would be 
> something external to the container which would proxy the request to your 
> application within the container.
> This patch enables external entities viz. zookeeper reach the host (based 
> on public_ip:public_port) from where a proxy will be required to route to the 
> application running within the container. If we use , zookeeper 
> won't be able to reach. The setting up of the proxy is not a part of docker 
> or libprocess.
> Please also refer to relevant issue 
> https://issues.apache.org/jira/browse/MESOS-2587.

Can we get some inputs on this so as to move forward?
Please also look into https://reviews.apache.org/r/34129/ which is also a part 
of this fix.


- Anindya


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


On May 18, 2015, 10:08 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34128/
> ---
> 
> (Updated May 18, 2015, 10:08 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-809
> https://issues.apache.org/jira/browse/MESOS-809
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Expose environment variables LIBPROCESS_PUBLIC_IP and LIBPROCESS_PUBLIC_PORT 
> as the IP and
> port which libprocess would advertise (if set). If not set, it defaults to
> the IP and port on which it binded to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> e3de3cd6b536aaaf59784360aed546512dd04dc9 
> 
> Diff: https://reviews.apache.org/r/34128/diff/
> 
> 
> Testing
> ---
> 
> Testing:
>   make test
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 36547: Fixed fetcher failing for FTP URIs.

[Adam B]

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



src/launcher/fetcher.cpp (lines 127 - 128)


Why not just check for any 2xx code then? Aren't they all successful in one 
way or another?


- Adam B


On July 16, 2015, 9:55 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36547/
> ---
> 
> (Updated July 16, 2015, 9:55 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Joerg Schad.
> 
> 
> Bugs: MESOS-3060
> https://issues.apache.org/jira/browse/MESOS-3060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The response code for successful FTP file transfers is 226, while it is 200 
> for HTTP. The fetcher has been changed to check for a response code of 226 
> for FTP URIs.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd 
> 
> Diff: https://reviews.apache.org/r/36547/diff/
> 
> 
> Testing
> ---
> 
> make check & external FTP server test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36464: Updated scheduler driver to send ACCEPT call.

[Vinod Kone]

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

(Updated July 17, 2015, 3:14 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's. NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 
  src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36467: Updated scheduler driver to send ACKNOWLEDGE call.

[Vinod Kone]

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

(Updated July 17, 2015, 3:16 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 
  src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e 
  src/tests/reconciliation_tests.cpp 6042d8c02d86f486e0c4d82d5a70666d7ac9019b 
  src/tests/scheduler_tests.cpp 2ce280a9b153263130694820c101cbad29471179 
  src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 
  src/tests/slave_tests.cpp 89cc7f68b33b037626ca6056647c360b5a6d5901 
  src/tests/status_update_manager_tests.cpp 
440b07475e28dc491ab640acaca8ee20db8411f8 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36469: Updated scheduler driver to send MESSAGE call.

[Vinod Kone]

> On July 15, 2015, 5:58 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1313-1315
> > 
> >
> > We don't need to worry about gcc 4.1.* anymore, you can assign now on 
> > the same line :)

copy paste error. fixed.


> On July 15, 2015, 5:58 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, line 1265
> > 
> >
> > Did you want to expect that the message is sent to through the master 
> > using call, since it looks like no offers go to the second scheduler?

sure thing.


- Vinod


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


On July 14, 2015, 12:30 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36469/
> ---
> 
> (Updated July 14, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d 
>   src/tests/fault_tolerance_tests.cpp 
> 1070ccf17f98f6b3800684a5edd6517d90631c3e 
> 
> Diff: https://reviews.apache.org/r/36469/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36469: Updated scheduler driver to send MESSAGE call.

[Vinod Kone]

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

(Updated July 17, 2015, 3:16 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's. NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 
  src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e 
  src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36470: Updated scheduler driver to send TEARDOWN call.

[Vinod Kone]

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

(Updated July 17, 2015, 3:18 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's. NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 
  src/tests/exception_tests.cpp 9af16748ae67671bd327a1ea7c946a7a38c5f7ff 
  src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e 
  src/tests/master_allocator_tests.cpp 534b2486a6a02ef32b5a7235d3ad30e82a78d6a5 
  src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc 
  src/tests/rate_limiting_tests.cpp 49d907b0be45ccfed8af5c8fda89ad560e012c1e 
  src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36470: Updated scheduler driver to send TEARDOWN call.

[Vinod Kone]

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

(Updated July 17, 2015, 5:35 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

rebased. NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 
  src/tests/exception_tests.cpp 9af16748ae67671bd327a1ea7c946a7a38c5f7ff 
  src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e 
  src/tests/master_allocator_tests.cpp 534b2486a6a02ef32b5a7235d3ad30e82a78d6a5 
  src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc 
  src/tests/rate_limiting_tests.cpp 49d907b0be45ccfed8af5c8fda89ad560e012c1e 
  src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36467: Updated scheduler driver to send ACKNOWLEDGE call.

[Vinod Kone]

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

(Updated July 17, 2015, 5:35 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

fixed the newly added test to use ACKNOWELDGE call. NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 
  src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e 
  src/tests/reconciliation_tests.cpp 6042d8c02d86f486e0c4d82d5a70666d7ac9019b 
  src/tests/scheduler_event_call_tests.cpp 
cf6aa19a644580ff9d805e919763e9342d72677f 
  src/tests/scheduler_tests.cpp 2ce280a9b153263130694820c101cbad29471179 
  src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 
  src/tests/slave_tests.cpp 89cc7f68b33b037626ca6056647c360b5a6d5901 
  src/tests/status_update_manager_tests.cpp 
440b07475e28dc491ab640acaca8ee20db8411f8 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 36424: Created a command executor helper method.

[Marco Massenzio]

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


>From an email thread 
>[here](http://www.mail-archive.com/dev@mesos.apache.org/msg32844.html):

> (1) Discard semantics: with subprocess the caller has the ability to kill
 the process through s.pid. However, with 'execute' that you've introduced,
 if the caller discards the future, we should be killing the process tree
 we've forked to clean up.

Nice catch - I may need some guidance on how to do that, but I'll give it a
shot.


> (2) As a general note, `Future>` as a type tends to be a bit
 confusing for folks as one has to resort to reading comments to figure out
 the difference between f.isFailed() and f.get().isError(). When we talk
 about "readability" we're generally including things like this, where it
 isn't obvious to the "reader" of the code. Similar to paul's approach, I'd
 suggest having a small struct to capture the data of a completed command
 (i.e. status, out, err) to make this very clear. e.g. `Future`
 Make sense?

Yes!
I was actually thinking about even creating a Protobuf for this (some kind
of CommandResultInfo) and use that.


> (3) Subprocess has two overloads, path+args (exec-style), and command (sh
> -c "command" style). Your documentation says "sh -c" but the code is just
> directly passing through to path+args subprocess, so it's not going through
> sh -c at all. Something I'm missing?

Bad comment (I'd originally started down the 'sh -c' path, then figured out
it was not strictly necessary).
I'll fix that.


> Lastly, it would be great to see the caller code related to this, spot
 checking I see a number of places that are unnecessarily tedious (e.g.
 perf.cpp):
```
 // Start reading from stdout and stderr now. We don't use stderr
 // but must read from it to avoid the subprocess blocking on the
 // pipe.
 output.push_back(io::read(perf.get().out().get()));
 output.push_back(io::read(perf.get().err().get()));

 // Wait for the process to exit.
 perf.get().status()
   .onAny(defer(self(), &Self::_sample, lambda::_1));
```
> vs
```
 process::await(
 perf.get().status(),
 io::read(perf.get().out().get()),
 io::read(perf.get().err().get()))
   .then(defer(self(), &Self::_sample, lambda::_1));
```
> There is no need to wait for the command first before continuing, can just
 wait for everything. Having something like what you're proposing seems even
 cleaner, but it would be helpful to first start with the above improvement
 to get a better sense of all the use cases, and whether just adding a
 `.join()` that returns `Future` gets us all the way there. I propose
 this first because the structural simplifications needed will be very
 similar to those from `process::executeCommand`.

Uhm - this is assuming a deeper understanding of libprocess than I
currently have: let me ponder this a bit and I may reach out for more
advice.
And, yes, my code was largely based on `perf.cpp` blueprint.

- Marco Massenzio


On July 14, 2015, 4:21 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36424/
> ---
> 
> (Updated July 14, 2015, 4:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Cody Maloney.
> 
> 
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2902
> 
> While researching how to execute an arbitrary script
> to detect the Master IP address, it emerged clearly that
> a helper method to execute an arbitrary command/script on
> a node and obtain either stdout or stderr would have been
> useful and avoided a lot of code repetition.
> 
> This could not be ultimately used for the purpose at hand,
> but I believe it to be useful enough (particularly, to avoid
> people doing "coding by copy&paste" and/or waste time
> researching the same functionality).
> 
> This would also be beneficial in MESOS-2830, factoring out the remote command 
> execution logic.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> f6acb204582a9e696c3b09d4e4c543bb052e97d4 
> 
> Diff: https://reviews.apache.org/r/36424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 36424: Created a command executor helper method.

[Marco Massenzio]

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

(Updated July 17, 2015, 5:52 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, and Cody Maloney.


Changes
---

Added comments from Ben M.


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


Repository: mesos


Description (updated)
---

Jira: MESOS-2902

While researching how to execute an arbitrary script
to detect the Master IP address, it emerged clearly that
a helper method to execute an arbitrary command/script on
a node and obtain either stdout or stderr would have been
useful and avoided a lot of code repetition.

This could not be ultimately used for the purpose at hand,
but I believe it to be useful enough (particularly, to avoid
people doing "coding by copy&paste" and/or waste time
researching the same functionality).

This would also be beneficial in MESOS-2830 and MESOS-2834
factoring out the remote command execution logic.


Diffs
-

  3rdparty/libprocess/include/process/subprocess.hpp 
310cb4f8e4e2faa5545dffd196d7490c868bc5d6 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
f6acb204582a9e696c3b09d4e4c543bb052e97d4 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 36470: Updated scheduler driver to send TEARDOWN call.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36461, 36462, 36463, 36464, 36465, 36466, 36467, 36469, 36470]

All tests passed.

- Mesos ReviewBot


On July 17, 2015, 5:35 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36470/
> ---
> 
> (Updated July 17, 2015, 5:35 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2913
> https://issues.apache.org/jira/browse/MESOS-2913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 
>   src/tests/exception_tests.cpp 9af16748ae67671bd327a1ea7c946a7a38c5f7ff 
>   src/tests/fault_tolerance_tests.cpp 
> 1070ccf17f98f6b3800684a5edd6517d90631c3e 
>   src/tests/master_allocator_tests.cpp 
> 534b2486a6a02ef32b5a7235d3ad30e82a78d6a5 
>   src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc 
>   src/tests/rate_limiting_tests.cpp 49d907b0be45ccfed8af5c8fda89ad560e012c1e 
>   src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 
> 
> Diff: https://reviews.apache.org/r/36470/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 36450: Introduced Address and URL protobufs.

[Marco Massenzio]

> On July 15, 2015, 7:08 p.m., Vinod Kone wrote:
> > src/common/type_utils.cpp, line 131
> > 
> >
> > Is the order of query parameters important? Aren't these URLs 
> > equivalent?
> > 
> > http://a.b.c/?k1=a&k2=b
> > 
> > http://a.b.c/?k2=b&k1=a
> 
> Ben Mahler wrote:
> Java's URI class considers ordering as important:
> 
> 
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29
> 
> I'd also like to keep it simple for now, you'll notice that they consider 
> percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we 
> can just avoid this for now:
> 
> 
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29
> 
> Ideally, we'd have a URI / URL class in stout, where we can have a 
> comprehensive equality operator.
> 
> Anand Mazumdar wrote:
> +1 
> 
> There is already a URL struct that exists in libprocess that we might 
> consider enriching upon later and also move it to stout.
> 
> Ben Mahler wrote:
> I'll add a TODO related to this.

Interestingly enough, the original [RFC 
3986](https://tools.ietf.org/html/rfc3986)  (also mentioned 
[here](https://en.wikipedia.org/wiki/URL_normalization)) does not specify 
anything, leaving the matter to

>determination of equivalence or difference of URIs is based on string
   comparison, perhaps augmented by reference to additional rules
   provided by URI scheme definitions.
   
> (Sec. 6.1 Equivalence)

And the HTTP scheme, does not further clarify the matter; however, the only 
real difference seem to pertain to repeated query parameters, where the order 
*may* matter.

For my own curiosity, as the references were to Open JDK 6, I tried it also 
with the Oracle JDK 8 and they still made a string-wise comparison: 
```
URI uri = new URI("http://a.b.com?a=foo&b=bar";);
URI another = new URI("http://a.b.com?b=bar&a=foo";);
System.out.println(String.format("%s %s equal to %s",
uri,
uri.equals(another) ? "is" : "is not",
another));
```
yields:
```
http://a.b.com?a=foo&b=bar is not equal to http://a.b.com?b=bar&a=foo
```

I'm just noting this, because if we do decide (as it would seem reasonable and 
de facto adopted) to consider two URIs equivalent (regardless of the query 
params ordering) this should probably be noted in the equality operator 
documentation (to avoid someone tripping unwittingly on it).

Please be aware that URI is **different** from URL (the latter is a subset of 
the former).


- Marco


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


On July 17, 2015, 1:36 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36450/
> ---
> 
> (Updated July 17, 2015, 1:36 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-3012
> https://issues.apache.org/jira/browse/MESOS-3012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make the API more consistent, we'd like to have a single way to express a 
> network address.
> Also would like a way to express an HTTP address (a URL), which may include a 
> path prefix.
> 
> This also enables the message passing optimization in the scheduler driver 
> when receiving events, per MESOS-3012.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto d2f482668e671b30f2586f6aae9c20132ab4d1e4 
>   include/mesos/type_utils.hpp eb7fe2562cfcff52288d1c216425068d1ba551c0 
>   src/common/type_utils.cpp 2ad5b4cbe324c83e81fd7df7430652f5c0a4e30f 
>   src/master/master.cpp 082758ef54597ad32cf0d025c147f0f44dd11961 
>   src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc 
> 
> Diff: https://reviews.apache.org/r/36450/diff/
> 
> 
> Testing
> ---
> 
> Modified the simplest test I could find for offers.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>