Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

Review request for mesos, Alexander Rukletsov and Michael Park.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 7:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 1, 2015, 7:40 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Sept. 1, 2015, 9:39 a.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


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


Repository: mesos


Description
---

Added Non-Freezeer Task Killer.


Diffs (updated)
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Alexander Rukletsov]

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


That's awesome, Guangya, thanks for doing this!

Some high-level comments before we dive into wording:
* We recently adopted a new style for public headers, please update the 
comments according to it 
(https://mesos.apache.org/documentation/latest/mesos-doxygen-style-guide/)
* We definitely need one more native speaker to review the comments. I think 
Adam B (adam-mesos) can be a good fit. Mind adding him to the list of reviewers?

- Alexander Rukletsov


On Sept. 1, 2015, 7:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 1, 2015, 7:40 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 九月 1, 2015, 9:48 a.m., Alexander Rukletsov wrote:
> > That's awesome, Guangya, thanks for doing this!
> > 
> > Some high-level comments before we dive into wording:
> > * We recently adopted a new style for public headers, please update the 
> > comments according to it 
> > (https://mesos.apache.org/documentation/latest/mesos-doxygen-style-guide/)
> > * We definitely need one more native speaker to review the comments. I 
> > think Adam B (adam-mesos) can be a good fit. Mind adding him to the list of 
> > reviewers?

Thanks Alex, just add adam-mesos to review list, will update the RR based on 
the new style.


- Guangya


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


On 九月 1, 2015, 10:03 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 九月 1, 2015, 10:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Sept. 1, 2015, 12:14 p.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


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


Repository: mesos


Description
---

Added Non-Freezeer Task Killer.


Diffs (updated)
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Review Request 38003: MESOS-3351

[Klaus Ma]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

MESOS-3351 (duplicated slave id in master after master failover)


Diffs
-

  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 37996: Added property manager

[Alexander Rojas]

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

(Updated Sept. 1, 2015, 2:26 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Removes use of `auto` and for range loops.


Repository: mesos


Description
---

Introduces the `PathInheritedProperties` class which allows to create a tree 
where nodes can be tag with some property. This property is then inherited by 
children nodes.

Two behaviors are implemented, overriding, i.e. Adding a property to the child 
node of another node with a property already will result in the ancestor 
property being lost. The second behavior, accumulating, takes a function and 
accumulates
properties of all ancestors.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/Makefile.am 
f95ed03004d4e5382874d75969bc9285a0f44918 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
8853f92fcfcff81d0a3197bade02110685fa0325 
  3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
b1a519007e5bed196541f294c4676277f9708714 
  3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 37168: MESOS-3063

[Klaus Ma]

> On Sept. 1, 2015, 3:34 a.m., Michael Park wrote:
> > Just a bunch of high-level questions here.
> > 
> > It looks like you followed the pattern for 
> > `src/examples/test_framework.cpp`.
> > I think it would be probably better to follow 
> > `src/examples/persistent_volume_framework.cpp` since
> > 
> > 1. It's likely that people will visit dynamic reservation and persistent 
> > volumes together.
> > 2. There are many components (e.g. authorization, implicit status update 
> > acknowledgements, etc) included in the test_framework that are there for 
> > demo purposes. I think it would be better to keep this example's scope to 
> > dynamic reservation, and it should lead us to a simpler example.
> > 
> > Also, do we need the custom executor for some reason? It doesn't seem to be 
> > doing much currently?
> > 
> > Another high-level question is, how would you describe what `State` 
> > represents?
> > It seems like it's associated with every offer, would it makes more sense 
> > to associate it with a task instead?

1. Yes, I followed test_framework.cpp; and I'll polish it by following 
persistent_volume_framework.cpp.
2. Regarding the executor, we can use test_executor.cpp to replase.
3. For State, it describe whether a slave has reserved resources; it's an 
example for the daemon on each slave host, such as NameNode.

I'll update the code to address 1 & 2; if any more comments, i'll continue to 
address them :).


- Klaus


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


On Aug. 28, 2015, 3:31 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated Aug. 28, 2015, 3:31 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3063 (Add an example framework using dynamic reservation)
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff 
>   src/examples/dynamic_reservation_executor.cpp PRE-CREATION 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 3f56b30 
>   src/tests/flags.hpp 3644956 
>   src/tests/script.cpp bcc1fab 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38003: MESOS-3351

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38003]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 12:19 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38003/
> ---
> 
> (Updated Sept. 1, 2015, 12:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3351 (duplicated slave id in master after master failover)
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
> 
> Diff: https://reviews.apache.org/r/38003/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

[Alexander Rukletsov]

> On Aug. 31, 2015, 6:20 p.m., Michael Park wrote:
> > What do you think of the following API?
> > 
> > ```
> > JSON::Value value = ...;
> > auto message = protobuf::parse(value);  // message has type Try.
> > auto repeated = 
> > protobuf::parse>(value);  // repeated 
> > has type google::protobuf::RepeatedPtrField.
> > ```
> > 
> > This makes it so that `parse` always returns `Try`, which I think 
> > enables the use of `auto` for the result under our style guide.
> > I think this is a simpler pattern to remember than `parse` -> `Try` 
> > and `parseRepeated` -> `Try>`.
> > 
> > It would take `JSON::Value` rather than `JSON::Array` but check that it is 
> > indeed an instance of `JSON::Array`,
> > the same way the existing version takes `JSON::Value` and checks that it's 
> > an instance of `JSON::Object`.
> > 
> > It also makes the API symmetric with `read()`. We simply do `read` or 
> > `read>` and it does the right thing.
> 
> Michael Park wrote:
> __NOTE:__ `parseRepeated` is actually @joseph's suggestion in the 
> subsequent patch.
> 
> In this patch, `parse(value)` returns `Try` and `parse(array)` 
> returns `Try>`,
> which I think is just as hard if not harder to intuitively deduce.
> 
> What do you think?
> 
> Joseph Wu wrote:
> I think that would be good.  It seems more verbose, but also a lot more 
> explicit.
> 
> (But I can imagine that it might not fit in 80 characters in some 
> places...)
> 
> Michael Park wrote:
> > (But I can imagine that it might not fit in 80 characters in some 
> places...)
> 
> Yeah, but I think that's fine. It's the case with most uses of 
> `google::protobuf::RepeatedPtrField` across the codebase currently.
> We can also do `using google::protobuf::RepeatedPtrField;` in places 
> where possible.

I think it's a great idea, I'll go for the partial specialization trick.


- Alexander


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


On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Aug. 27, 2015, 3:38 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Sept. 1, 2015, 2:12 p.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


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


Repository: mesos


Description
---

Added Non-Freezeer Task Killer.


Diffs (updated)
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

[Alexander Rukletsov]

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

(Updated Sept. 1, 2015, 2:20 p.m.)


Review request for mesos, Joseph Wu and Michael Park.


Changes
---

Implemented MPark's suggestion.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

[Alexander Rukletsov]

> On Aug. 27, 2015, 5:51 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, lines 179-181
> > 
> >
> > From the method name, I think it's not completely obvious what the type 
> > is.
> > 
> > Since I wouldn't want to type 
> > `Try>` either, you could consider 
> > renaming to something like `protobuf::parseRepeated`.
> 
> Alexander Rukletsov wrote:
> We can do that. If we rename it to `parseRepeated()` we can even change 
> the signature to take `JSON::Object` as a parameter.

Decided to take another approach here, see previous review in the chain.


- Alexander


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


On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Aug. 27, 2015, 3:38 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> **NOTE**: Filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up 
> protobuf generation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

[Alexander Rukletsov]

> On Aug. 27, 2015, 2:22 a.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h, line 38
> > 
> >
> > Need adjust order here?
> 
> Alexander Rukletsov wrote:
> Not sure I follow, what exactly do you mean?
> 
> haosdent huang wrote:
> Oh, sorry for my english. I mean do we need ajust the order to
> ```
> class Message;
> class Nested;
> class SimpleMessage;
> ```

Oh, I see. You are right, we should have adjusted the order if this was "our" 
file. However, the file is generated by the `protoc`, hence no manual tinkering 
is expected. I have filed 
[MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to address the 
issue in the first place.


- Alexander


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


On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Aug. 27, 2015, 3:38 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> **NOTE**: Filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up 
> protobuf generation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

[Alexander Rukletsov]

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

(Updated Sept. 1, 2015, 2:25 p.m.)


Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
cfc2803e42284f641879fb24bce1282215c8ea52 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
a1d4084661345f9367c75f9db61279f032b93e69 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
bbd36d39e9588eb8eea6d739451ad3bab029ca08 

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


Testing
---

make check (Mac OS 10.10.4)

**NOTE**: Filed [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) 
to clean up protobuf generation.


Thanks,

Alexander Rukletsov

Re: Review Request 37830: Added a test for converting JSON arrays to Resources.

[Alexander Rukletsov]

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

(Updated Sept. 1, 2015, 2:27 p.m.)


Review request for mesos, Joseph Wu and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

[haosdent huang]

> On Aug. 27, 2015, 2:22 a.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h, line 38
> > 
> >
> > Need adjust order here?
> 
> Alexander Rukletsov wrote:
> Not sure I follow, what exactly do you mean?
> 
> haosdent huang wrote:
> Oh, sorry for my english. I mean do we need ajust the order to
> ```
> class Message;
> class Nested;
> class SimpleMessage;
> ```
> 
> Alexander Rukletsov wrote:
> Oh, I see. You are right, we should have adjusted the order if this was 
> "our" file. However, the file is generated by the `protoc`, hence no manual 
> tinkering is expected. I have filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to address the 
> issue in the first place.

Oh, sorry. I not realize it is generated by protobuf before.


- haosdent


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


On Sept. 1, 2015, 2:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Sept. 1, 2015, 2:25 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> **NOTE**: Filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up 
> protobuf generation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38003: MESOS-3351

[Klaus Ma]

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

(Updated Sept. 1, 2015, 2:44 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Add JIRA link


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


Repository: mesos


Description
---

MESOS-3351 (duplicated slave id in master after master failover)


Diffs
-

  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36612, 36620]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 2:12 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Sept. 1, 2015, 2:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 1, 2015, 2:59 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37958: Add an option to only perform batch allocations.

[Alexander Rukletsov]

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

Ship it!


LGTM. I would love us to avoid creating a master flag for each allocator 
option, mind creating a JIRA for this?

- Alexander Rukletsov


On Aug. 31, 2015, 4:49 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Aug. 31, 2015, 4:49 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp b2160f50db3e0b0d04b0625e7589004016c0f746 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37830: Added a test for converting JSON arrays to Resources.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37826, 37827, 37830]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 2:27 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37830/
> ---
> 
> (Updated Sept. 1, 2015, 2:27 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
> 
> Diff: https://reviews.apache.org/r/37830/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 1, 2015, 2:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37958: Add an option to only perform batch allocations.

[James Peach]

> On Sept. 1, 2015, 2:01 a.m., Guangya Liu wrote:
> > docs/configuration.md, line 292
> > 
> >
> > The following format may be better
> > 
> >   If 'true' xxx (default: true)
> >   
> >   If 'false' xxx 
> > 

I don't think that the `true`/`false` language makes this easier to read. I did 
update the language slightly and I hope that it clarifies the behaviour.


- James


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


On Aug. 31, 2015, 4:49 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Aug. 31, 2015, 4:49 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp b2160f50db3e0b0d04b0625e7589004016c0f746 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37958: Add an option to only perform batch allocations.

[James Peach]

> On Sept. 1, 2015, 12:18 a.m., Qian Zhang wrote:
> > include/mesos/master/allocator.hpp, line 48
> > 
> >
> > I think we do not want "{}" in the newline.

Fixed.


> On Sept. 1, 2015, 12:18 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 398
> > 
> >
> > Tab here? If so, we may need to replace tab with spaces.

AFAICT these are spaces.


> On Sept. 1, 2015, 12:18 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 397
> > 
> >
> > Instead of checking allocationOptions.allocateOnEvents everywhere, can 
> > we just do it once internally in allocate()?

The problem with doing it in allocate() is that allocate() doesn't know whether 
it is being called in response to a cluster event, or by the periodic 
allocation cycle. We could pass down a flag or wrap it in some other way, but 
that did not seem any less clumsy.


- James


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


On Aug. 31, 2015, 4:49 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Aug. 31, 2015, 4:49 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp b2160f50db3e0b0d04b0625e7589004016c0f746 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37958: Add an option to only perform batch allocations.

[James Peach]

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

(Updated Sept. 1, 2015, 4:34 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebased onto master and addressed some review comments.


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


Repository: mesos


Description
---

Normally, the mesos-master performs allocation cycles in response
to protocol messages and cluster events. This ties the cost of
allocation to the amount of churn in the environment, which can led
to unpredictable performance. Add an option to disable this, meaning
that allocations are only every performed by the batch interval
task. This makes resource allocation a fixed cost, independent of
the amount of environmental churn.


Diffs (updated)
-

  docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
  src/master/allocator/mesos/allocator.hpp 
aa55755a9c3250579e9366bdbc17a2449e95d659 
  src/master/allocator/mesos/hierarchical.hpp 
38f8fd2c84314bb3731684d0e9795cb4f50a227e 
  src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
  src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/tests/hierarchical_allocator_tests.cpp 
9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
  src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 

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


Testing
---

"make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
weeks.


Thanks,

James Peach

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

[Joseph Wu]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 551 - 553)


Can you explain why this is a struct rather than a function?


- Joseph Wu


On Sept. 1, 2015, 7:20 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 1, 2015, 7:20 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Alexander Rukletsov]

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



include/mesos/master/allocator.hpp (lines 62 - 64)


I think we can kill this for brevity.



include/mesos/master/allocator.hpp (line 68)


Period in the end, please!



include/mesos/master/allocator.hpp (line 70)


I think we tend to merge detailed description with the summary in case both 
are very short. Here and below.

Also, let's mention that calling `initialize()` supposes to start 
allocation process.



include/mesos/master/allocator.hpp (line 72)


Please a period at the end!



include/mesos/master/allocator.hpp (line 87)


I think we prefer 's' at the end of the verb in such cases: `Add*s* a 
framework`.



include/mesos/master/allocator.hpp (line 89)


I think "register" is misleading. Allocator is notified that a new 
framework joins the cluster and is entitled to participate in resource sharing.



include/mesos/master/allocator.hpp (lines 105 - 106)


It's an interface, it cannot guarantee that resoruces will be released. We 
should document an expectation or a contract here. It's up to an allocator what 
to do in this case, the built-in just removes the framework from the fair share 
pool AFAIK. Let's reword.



include/mesos/master/allocator.hpp (line 108)


Let's capiralize `Id` for clarity. Here and everywhere.



include/mesos/master/allocator.hpp (line 117)


s/activated/active



include/mesos/master/allocator.hpp (lines 138 - 141)


This it true for the built-in allocator, but not necessarily for *any* 
allocator. Could you please reword putting an accent on *under what 
cercumstances* the method is called and maybe what an expected behaviour may be?



include/mesos/master/allocator.hpp (lines 154 - 158)


The comment about static reservations is important, let's keep it in the 
description. Again, let's add some information on when it's called.



include/mesos/master/allocator.hpp (line 170)


We try to switch to "agent" instead of "slave". Let's do it in the comments 
(here and everywhere). Also, let's have a note in the beginning of the doc 
saying "agent" is the new "slave".



include/mesos/master/allocator.hpp (line 212)


I think we remove only outstanding offers, right?



include/mesos/master/allocator.hpp (line 213)


... and resources recovered in a separate call.



include/mesos/master/allocator.hpp (line 221)


I think "agent" is more clear than "host" here. You maybe refered to the 
fact that the whitelist consists of hostnames, but that's slightly different 
and should be documented in the whitelist class.



include/mesos/master/allocator.hpp (line 225)


I don't think this is correct. AFAIK whitelist contains slaves that should 
not participate in the allocation, basically, they are deactivated.



include/mesos/master/allocator.hpp (lines 235 - 237)


Let's say here that a framework may request resources, but it is up to 
allocator whether and how to satisfy this request.



include/mesos/master/allocator.hpp (lines 280 - 283)


Also, when framework is deactivated.


- Alexander Rukletsov


On Sept. 1, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 1, 2015, 2:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> 

Re: Review Request 37958: Add an option to only perform batch allocations.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37958]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 4:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Sept. 1, 2015, 4:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37585: Maintenance primitives: Add a user doc.

[Joseph Wu]

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

(Updated Sept. 1, 2015, 10:42 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
Joris Van Remoortere, and Vinod Kone.


Changes
---

Updated example JSON and CURL based on current MVP.  Updated picture.


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


Repository: mesos


Description
---

Heavily based on the design doc 
(https://docs.google.com/document/d/16k0lVwpSGVOyxPSyXKmGC-gbNmRlisNEe4p-fAUSojk/).

Includes a diagram of the maintenance mode transitions.

This documents the current working prototype (MVP), which starts here:
https://reviews.apache.org/r/36321/

One TODO remaining: Update with the release version that maintenance primitives 
will be released in.


Diffs (updated)
-

  docs/images/maintenance-primitives-modes.png PRE-CREATION 
  docs/maintenance.md PRE-CREATION 

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


Testing
---

Copied to: https://gist.github.com/kaysoky/b9789c88ee204e3b49a2
Checked for markdown correctness.


File Attachments (updated)


Same as the image in the binary diff. (Uploaded for reviewer convenience.)
  
https://reviews.apache.org/media/uploaded/files/2015/09/01/7d3153ca-37f4-4948-acce-b140a3eb71a9__maintenance-primitives-modes.png


Thanks,

Joseph Wu

Review Request 38011: Maintenance Primitives: Use the parse> helper instead of a plural MachineID protobuf.

[Joseph Wu]

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

Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Artem 
Harutyunyan, Joris Van Remoortere, and Michael Park.


Repository: mesos


Description
---

This removes the MachineIDs protobuf and changes it to instead use the 
RepeatedFieldPtr.
This also changes the maintenance primitives (alpha) API for /machine/up and 
/machine/down, which both take an array instead of an object now.


Diffs
-

  include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
  include/mesos/v1/mesos.proto fdd678b329bd15b412d9460d24d29387d2945630 
  src/common/protobuf_utils.hpp 86474eac116306e50f276bc0539de9bca66133e0 
  src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46 
  src/master/http.cpp 94e97a2898106579434e8cdec04b7b0e130a810e 
  src/master/maintenance.hpp bebaeb24efbd58df38796e3941f36a21791b6bfd 
  src/master/maintenance.cpp 277dd8270fbd497616e5b24db4ea7dd615e170e3 
  src/tests/master_maintenance_tests.cpp 
fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
  src/tests/registrar_tests.cpp aa49c86c94446f17f705039075050296b4af1955 

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


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 37958: Add an option to only perform batch allocations.

[Ben Mahler]

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

Ship it!


Just some minor updates and we can get this committed. Thanks James!

Let's be clear in the description that this is motivated by some performance 
bugs in which event-based allocation will enqueue too many allocation rounds, 
and the allocator's performance is degraded due to the allocator's queue 
getting backed up. "Unpredictable performance" might not be clear to those who 
encounter this later :)


docs/configuration.md (lines 291 - 295)


Can you have this match the help message from flags.cpp? In particular, 
let's not tell people that this gives more predictable performance, as that is 
alluding to the fact that there is are performance bug(s) in the allocator, and 
we should fix those!



include/mesos/master/allocator.hpp (lines 43 - 56)


How about a 'struct' with no default constructor? Putting in these default 
values is a bit odd, since these fields should be coming from the master flags.



include/mesos/master/allocator.hpp (line 81)


How about s/allocationOptions/options/ here and everywhere else in this 
patch?



src/master/allocator/mesos/hierarchical.hpp (line 282)


Can you move this back up to below 'initialized' for now?


- Ben Mahler


On Sept. 1, 2015, 4:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Sept. 1, 2015, 4:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37585: Maintenance primitives: Add a user doc.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37585]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 5:42 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37585/
> ---
> 
> (Updated Sept. 1, 2015, 5:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2083
> https://issues.apache.org/jira/browse/MESOS-2083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Heavily based on the design doc 
> (https://docs.google.com/document/d/16k0lVwpSGVOyxPSyXKmGC-gbNmRlisNEe4p-fAUSojk/).
> 
> Includes a diagram of the maintenance mode transitions.
> 
> This documents the current working prototype (MVP), which starts here:
> https://reviews.apache.org/r/36321/
> 
> One TODO remaining: Update with the release version that maintenance 
> primitives will be released in.
> 
> 
> Diffs
> -
> 
>   docs/images/maintenance-primitives-modes.png PRE-CREATION 
>   docs/maintenance.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37585/diff/
> 
> 
> Testing
> ---
> 
> Copied to: https://gist.github.com/kaysoky/b9789c88ee204e3b49a2
> Checked for markdown correctness.
> 
> 
> File Attachments
> 
> 
> Same as the image in the binary diff. (Uploaded for reviewer convenience.)
>   
> https://reviews.apache.org/media/uploaded/files/2015/09/01/7d3153ca-37f4-4948-acce-b140a3eb71a9__maintenance-primitives-modes.png
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Ben Mahler]

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


Some notes before you rebase.


src/linux/perf.cpp (line 490)


Why not make this an Option and when it's  this the 
value is None?



src/linux/perf.cpp (line 498)


How about 'parse' to be consistent with how we've named these kinds of 
functions, perhaps making it a static member of Sample?

No need for inline here?



src/linux/perf.cpp (lines 500 - 512)


Indentation is off here, please have a look over the diff, also s/if(/if (/ 
:)



src/linux/perf.cpp (lines 501 - 504)


Don't forget to update this when you rebase.



src/linux/perf.cpp (line 511)


The caller is expected to print the line when composing the error message, 
the callee here should print why the line is bad.



src/linux/perf.cpp (lines 522 - 524)


Let's include the line contents in the error message by composing a message 
with the parse error:

```
return Error("Failed to parse perf sample line '" + line + "': " + 
parse.error());
```



src/linux/perf.cpp (lines 531 - 577)


Can you now use the -> operator here? Might also avoid the need to wrap 
these, although, the "" logic should be moved to 'parse()'.



src/linux/perf.cpp (lines 540 - 543)


Now that you have a parse function, let's put the  logic 
there as well.


- Ben Mahler


On Aug. 31, 2015, 10:33 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37442/
> ---
> 
> (Updated Aug. 31, 2015, 10:33 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Factor out the token extraction rules in prepartion for extending them to 
> cope with multiple versions.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
> 
> Diff: https://reviews.apache.org/r/37442/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 37462: Add support for version detection and parsing.

[Paul Brett]

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

(Updated Sept. 1, 2015, 7:47 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Updated to reflect removal of PID sampling.


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


Repository: mesos


Description
---

Add support for version detection and parsing.


Diffs (updated)
-

  src/linux/perf.hpp c5630118f4858b1a805f25a2db7a24ca0989 
  src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee 
  src/tests/containerizer/perf_tests.cpp 
8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 

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


Testing
---

sudo make check

Perf tests may fail on many machines because the tests are currently using the 
version of perf installed on the machine to choose decode.  The next patch will 
extend the perf tests to supply test cases for each of the supported perf 
versions.


Thanks,

Paul Brett

Re: Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Paul Brett]

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

(Updated Sept. 1, 2015, 7:49 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Factor out the token extraction rules in prepartion for extending them to cope 
with multiple versions.


Diffs (updated)
-

  src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 38011: Maintenance Primitives: Use the parse> helper instead of a plural MachineID protobuf.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37826, 37827, 37830, 38011]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 5:44 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38011/
> ---
> 
> (Updated Sept. 1, 2015, 5:44 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Artem 
> Harutyunyan, Joris Van Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes the MachineIDs protobuf and changes it to instead use the 
> RepeatedFieldPtr.
> This also changes the maintenance primitives (alpha) API for /machine/up and 
> /machine/down, which both take an array instead of an object now.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   include/mesos/v1/mesos.proto fdd678b329bd15b412d9460d24d29387d2945630 
>   src/common/protobuf_utils.hpp 86474eac116306e50f276bc0539de9bca66133e0 
>   src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46 
>   src/master/http.cpp 94e97a2898106579434e8cdec04b7b0e130a810e 
>   src/master/maintenance.hpp bebaeb24efbd58df38796e3941f36a21791b6bfd 
>   src/master/maintenance.cpp 277dd8270fbd497616e5b24db4ea7dd615e170e3 
>   src/tests/master_maintenance_tests.cpp 
> fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
>   src/tests/registrar_tests.cpp aa49c86c94446f17f705039075050296b4af1955 
> 
> Diff: https://reviews.apache.org/r/38011/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

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

Ship it!


Ship It!


src/slave/containerizer/provisioners/docker/token_manager.hpp (line 38)


Let's remove the provisioners namespace to be at the same level with appc 
provisioner



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 207)


You should move the Process into the cpp file if you don't plan to inherit 
this for mocking.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 61)


Why not just take a copied value, so
just [](string segment)?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 299)


Align this next to ( :

return Failure("Failed to parse JSON Web .." + 
   token.error());



src/tests/provisioners/docker_provisioner_tests.cpp (line 341)


I can't tell what's invalid about this?


- Timothy Chen


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Paul Brett]

> On Sept. 1, 2015, 7:09 p.m., Ben Mahler wrote:
> > src/linux/perf.cpp, line 490
> > 
> >
> > Why not make this an Option and when it's  this 
> > the value is None?

Possible values are uint64_t, double, "" and "".  
I'm not sure how useful it is to distingish between the last two but choosing 
between uint64_t and double cannot be done until we lookup the event.


- Paul


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


On Sept. 1, 2015, 7:49 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37442/
> ---
> 
> (Updated Sept. 1, 2015, 7:49 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Factor out the token extraction rules in prepartion for extending them to 
> cope with multiple versions.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee 
> 
> Diff: https://reviews.apache.org/r/37442/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 37853: Overlay filesystem provisioning backend

[Mei Wan]

> On Aug. 27, 2015, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/overlay.cpp, lines 101-103
> > 
> >
> > Hum, I'd like to understand why overlay backend cannot support 1 layer. 
> > Any reason?

The mount options for overlayfs either takes one lowerdir, one upperdir and one 
workdir, or at least 2 lowerdirs. It's just how the function works.


> On Aug. 27, 2015, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/overlay.cpp, line 111
> > 
> >
> > I think you should be able to use strings::join here:
> > 
> > ```
> > strings::join(":", layers);
> > ```

The format would be dir1:dir2:dir3 etc. I'm just wondering is it possible to 
have the semi colon only appear between the directories? If I'm thinking about 
this correctly, strings::join will give me :dir1:dir2:dir3? Also since I'm 
stacking the layers in reverse from the comment above, does this still apply?


> On Aug. 27, 2015, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/overlay.cpp, lines 110-114
> > 
> >
> > Could you please add a comment about the ordering the layers will be 
> > stacked.
> > 
> > I think we should add a document at the backend interface as well 
> > stating that layers are stacking in such a way that the front layer in the 
> > vector will be the bottom most layer (i.e., applied first).
> > 
> > The lowerdir here should actually reverse the order of the layers 
> > vector.
> > 
> > ```
> > The specified lower directories will be stacked beginning from the 
> > rightmost one and going left.  In the above example lower1 will be the top, 
> > lower2 the middle and lower3 the bottom layer.
> > ```

Where would I add this document? I could just add it to the flag?


> On Aug. 27, 2015, 10:42 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/overlay.cpp, line 58
> > 
> >
> > You also want to check if overlay fs is supported or not. Not every 
> > linux kernel supports overlay fs.

How would I check this?


- Mei


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


On Aug. 27, 2015, 9:11 p.m., Mei Wan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37853/
> ---
> 
> (Updated Aug. 27, 2015, 9:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Jie Yu.
> 
> 
> Bugs: MESOS-2971
> https://issues.apache.org/jira/browse/MESOS-2971
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the overlay filesystem backend by layering the images as a 
> read-only filesystem.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335 
>   src/slave/containerizer/provisioners/backends/overlay.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/overlay.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37853/diff/
> 
> 
> Testing
> ---
> 
> I haven't done any official testing. When I was working off Ian's branch, I 
> tested it manually and the provisioning works.
> 
> 
> Thanks,
> 
> Mei Wan
> 
>

Re: Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Paul Brett]

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

(Updated Sept. 1, 2015, 8:42 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Incorporate some of Ben's feedback.  Does not implement decoding of the Sample 
value field in Sample::parse.


Repository: mesos


Description
---

Factor out the token extraction rules in prepartion for extending them to cope 
with multiple versions.


Diffs (updated)
-

  src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 37462: Add support for version detection and parsing.

[Paul Brett]

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

(Updated Sept. 1, 2015, 8:45 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add support for version detection and parsing.


Diffs (updated)
-

  src/linux/perf.hpp c5630118f4858b1a805f25a2db7a24ca0989 
  src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee 
  src/tests/containerizer/perf_tests.cpp 
8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 

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


Testing
---

sudo make check

Perf tests may fail on many machines because the tests are currently using the 
version of perf installed on the machine to choose decode.  The next patch will 
extend the perf tests to supply test cases for each of the supported perf 
versions.


Thanks,

Paul Brett

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 61)


Its effectively the same. const reference as method argument is just a good 
practice, thats all.


- Jojy Varghese


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

> On Aug. 31, 2015, 8:09 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 131
> > 
> >
> > For local store I don't think this comment is valid.
> > Should just comment that only local images with file:// is supported.

The implemenation gets changed dramatically in the commit 37200 (refactoring 
docker image struct) so are these comments here still relevant?


> On Aug. 31, 2015, 8:09 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 247
> > 
> >
> > If tar is terminated in the middle, will this operation succeed again? 
> > We only check the directory exist to signal that we untarred the layer, but 
> > it might not necessarily really finished.
> > I Think we will have to do a mv here as well

So untar into a different temp directory and rename it to appear in the staging 
directory?


- Lily


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Anand Mazumdar]

> On Sept. 1, 2015, 9:13 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 61
> > 
> >
> > Its effectively the same. const reference as method argument is just a 
> > good practice, thats all.

Can you elaborate a bit more on what does the const reference buys us here 
since you end up removing the const-ness later ? 

Anyways if you still want to keep it why not name it as a continuation variable 
? Something similar to this:

string segment_(segment); // Remove const.


- Anand


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


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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



src/tests/provisioners/docker_provisioner_tests.cpp (line 341)


URL is cooked (not real).


- Jojy Varghese


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)


Remove provisioners namespace for now



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)


Remove provisioners namespace for now



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 50)


spell out credentials



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 67)


ditto



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 93)


fix ident (4 spaces, so move 2 spaces left)



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 108)


We should move all the logic into process, see similiar comment made in 
AppProvisionerProcess 
(https://reviews.apache.org/r/37881/diff/2?file=1059946#file1059946line174)



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 186)


What's the benefits for this inline lambda vs just putting this code in the 
foreach loop?

I don't see you reuse it mulitple places and there is only one single call 
site.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 214)


Can you add some verbose logging especially when we're calling ourselves 
again?

This seems like code that we can get into trouble especially when the 
docker registry implementation changes (ie: they start returning 202 instead of 
200, or infinite recirusion starts happening, etc).



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 219)


This should be Option right? Since we might not have a 
lastResponseStatus. And we can default it to None() so all the callers for this 
method can just skip that instead of passing in a ""

Can you also comment on why we need this?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 233)


You should state the assumption in a resend that we don't expect a response 
status that causes another resend, which can still cause infinitte recursion.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 235)


You did "Invalid Response :" + error in the bottom, and "Invalid Response 
'" + error + "'" here.

Let's just keep the same format, I suggest the former.

Also I think worth mentioning that this is matching the last response.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 266)


The identation seems off, I think you should probably just move the self() 
to the last line and the beginning of lambda too.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 281)


Can you also add a comment that we need to add redirect functionality into 
libprocess too? Once we have that we don't have to handle it ourselves



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 291)


strings::contains



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 293)


Fix alignment



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 306)


Put this in a constant.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 309)


Can you comment what's this block is for?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 315)


Fix indentation.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 336)


Fix alignment



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 359)


Should we make sure somewhere that we encode or check the tag so that we 
don't contain spaces?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 382)


Move thi

Re: Review Request 37958: Add an option to only perform batch allocations.

[James Peach]

> On Sept. 1, 2015, 6 p.m., Ben Mahler wrote:
> > include/mesos/master/allocator.hpp, lines 43-56
> > 
> >
> > How about a 'struct' with no default constructor? Putting in these 
> > default values is a bit odd, since these fields should be coming from the 
> > master flags.

This is needed for uses that don't get flags from the master (eg. tests).


- James


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


On Sept. 1, 2015, 4:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Sept. 1, 2015, 4:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37466: Update perf tests to including testing the supported perf output formats.

[Paul Brett]

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

(Updated Sept. 1, 2015, 10:05 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebase and remove PID tests.


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


Repository: mesos


Description
---

Update perf tests to including testing the supported perf output formats.


Diffs (updated)
-

  src/tests/containerizer/perf_tests.cpp 
8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 37466: Update perf tests to including testing the supported perf output formats.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [37416, 37442]

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

Error:
 2015-09-01 22:18:04 URL:https://reviews.apache.org/r/37442/diff/raw/ 
[4003/4003] -> "37442.patch" [1]
error: patch failed: src/linux/perf.cpp:378
error: src/linux/perf.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Sept. 1, 2015, 10:05 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> ---
> 
> (Updated Sept. 1, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update perf tests to including testing the supported perf output formats.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/perf_tests.cpp 
> 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 
> 
> Diff: https://reviews.apache.org/r/37466/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 108)


Why add the heavy lifting of process dispatch when we can avoid it? Wont be 
efficient.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 382)


Will exceed 80 chars.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 410)


I can add a todo/jira.


- Jojy Varghese


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 186
> > 
> >
> > What's the benefits for this inline lambda vs just putting this code in 
> > the foreach loop?
> > 
> > I don't see you reuse it mulitple places and there is only one single 
> > call site.

its more functional to do it this way.


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 214
> > 
> >
> > Can you add some verbose logging especially when we're calling 
> > ourselves again?
> > 
> > This seems like code that we can get into trouble especially when the 
> > docker registry implementation changes (ie: they start returning 202 
> > instead of 200, or infinite recirusion starts happening, etc).

We only explicitly handle certain status codes. We can expand the logci if 
required but currently its very strict. Also, how can it get into infinite 
recursion?


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 233
> > 
> >
> > You should state the assumption in a resend that we don't expect a 
> > response status that causes another resend, which can still cause infinitte 
> > recursion.

Could you explain the case where it can cause infinite recursion?


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > 
> >
> > Should we make sure somewhere that we encode or check the tag so that 
> > we don't contain spaces?

I am not sure if http path can contain spaces. Queries can.


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37908: Silence oversubscription logging

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Aug. 31, 2015, 6:28 p.m., Niklas Nielsen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37908/
> ---
> 
> (Updated Aug. 31, 2015, 6:28 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resource estimates are continuously written to the logs even though they are 
> empty (as with the majority of Mesos clusters at the moment). To reduce the 
> noise, this patch moves log lines for empty resources to VLOG(1).
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8cfad7b86675b10c98524174975828bbcf47aed2 
> 
> Diff: https://reviews.apache.org/r/37908/diff/
> 
> 
> Testing
> ---
> 
> make check - with and without the patch, verifying the log statements.
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Re: Review Request 37785: Fix Flaky SlaveTest.HTTPSchedulerSlaveRestart test

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Aug. 26, 2015, 3:07 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37785/
> ---
> 
> (Updated Aug. 26, 2015, 3:07 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-3311
> https://issues.apache.org/jira/browse/MESOS-3311
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> I was not able to reproduce this with 300 gtest iterations in a loop on a 
> Ubuntu 14.04 VM with clang + ssl i.e. similar to the ASF setup.
> 
> The logs though made it pretty evident on what was going on. The slave was 
> sending a retry re-register message to the master, resulting in the master 
> sending back another FrameworkUpdateMessage, the 2nd one used to set the PID 
> from None() to the original pid() making the message go through directly to 
> the scheduler instead of being routed through the master.
> 
> Log Lines:
> 
> I0825 22:07:39.085610 27642 slave.cpp:1209] Will retry registration in 
> 6.014445ms if necessary
> I0825 22:07:39.092914 27640 master.cpp:3773] Re-registering slave 
> 20150825-220736-234885548-51219-27610-S0 at slave(286)@172.17.0.14:51219 
> (09c6504e3a31)
> I0825 22:07:39.093181 27630 slave.cpp:1209] Will retry registration in 
> 20.588077ms if necessary
>  some lines and then
> I0825 22:07:39.094435 27640 master.cpp:3773] Re-registering slave 
> 20150825-220736-234885548-51219-27610-S0 at slave(287)@172.17.0.14:51219 
> (09c6504e3a31)
> ... more lines
> I0825 22:07:39.096372 27635 slave.cpp:2131] Updating framework 
> 20150825-220736-234885548-51219-27610- pid to @0.0.0.0:0
> ... more lines
> I0825 22:07:39.097450 27635 slave.cpp:2131] Updating framework 
> 20150825-220736-234885548-51219-27610- pid to 
> scheduler-6c5ddcdb-9dd1-4b38-b051-5f714d3c1c55@172.17.0.14:51219
> ... more lines
> I0825 22:07:39.098433 27635 slave.cpp:3043] Sending message for framework 
> 20150825-220736-234885548-51219-27610- to 
> scheduler-6c5ddcdb-9dd1-4b38-b051-5f714d3c1c55@172.17.0.14:51219
> 
> 
> Paused the clock and then settle/resume invocations to ensure the retry does 
> not happen
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp d55e9dd4f4eb84a8fda85439e31a38e70890b377 
> 
> Diff: https://reviews.apache.org/r/37785/diff/
> 
> 
> Testing
> ---
> 
> make check again with 300 iterations without failure
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 38023: Rename for agent in user doc

[Isabel Jimenez]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Rename for agent in user doc for new scheduler HTTP API


Diffs
-

  docs/scheduler_http_api.md 11f4d83 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Sept. 1, 2015, 9:13 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 61
> > 
> >
> > Its effectively the same. const reference as method argument is just a 
> > good practice, thats all.
> 
> Anand Mazumdar wrote:
> Can you elaborate a bit more on what does the const reference buys us 
> here since you end up removing the const-ness later ? 
> 
> Anyways if you still want to keep it why not name it as a continuation 
> variable ? Something similar to this:
> 
> string segment_(segment); // Remove const.

We dont remove the const'ness of the original string. The original string 
object is unchanged. So now we have to decide if we want to copy the object 
when calling the function or you want to copy the object inside the function. 
The choice made here is to keep the style of "passing by const reference" and 
do teh copy inside the function.


- Jojy


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


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38023: Rename for agent in user doc

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Sept. 1, 2015, 10:40 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38023/
> ---
> 
> (Updated Sept. 1, 2015, 10:40 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename for agent in user doc for new scheduler HTTP API
> 
> 
> Diffs
> -
> 
>   docs/scheduler_http_api.md 11f4d83 
> 
> Diff: https://reviews.apache.org/r/38023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38023: Rename for agent in user doc

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 九月 1, 2015, 10:40 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38023/
> ---
> 
> (Updated 九月 1, 2015, 10:40 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename for agent in user doc for new scheduler HTTP API
> 
> 
> Diffs
> -
> 
>   docs/scheduler_http_api.md 11f4d83 
> 
> Diff: https://reviews.apache.org/r/38023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Ben Mahler]

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

Ship it!


Will get this committed shortly.


src/linux/perf.cpp (lines 381 - 382)


This looks like the documentation for 'parse'?



src/linux/perf.cpp (lines 392 - 394)


Either? :)


- Ben Mahler


On Sept. 1, 2015, 8:42 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37442/
> ---
> 
> (Updated Sept. 1, 2015, 8:42 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Factor out the token extraction rules in prepartion for extending them to 
> cope with multiple versions.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee 
> 
> Diff: https://reviews.apache.org/r/37442/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 37958: Add an option to only perform batch allocations.

[James Peach]

> On Sept. 1, 2015, 6 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 284
> > 
> >
> > Can you move this back up to below 'initialized' for now?

Fixed.


> On Sept. 1, 2015, 6 p.m., Ben Mahler wrote:
> > include/mesos/master/allocator.hpp, line 81
> > 
> >
> > How about s/allocationOptions/options/ here and everywhere else in this 
> > patch?

Renamed to ```allocator::Options```.


> On Sept. 1, 2015, 6 p.m., Ben Mahler wrote:
> > docs/configuration.md, lines 291-295
> > 
> >
> > Can you have this match the help message from flags.cpp? In particular, 
> > let's not tell people that this gives more predictable performance, as that 
> > is alluding to the fact that there is are performance bug(s) in the 
> > allocator, and we should fix those!

Done.


- James


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


On Sept. 1, 2015, 4:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Sept. 1, 2015, 4:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37958: Add an option to only perform batch allocations.

[James Peach]

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

(Updated Sept. 1, 2015, 11:10 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Updated with bmahler's review comments.


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


Repository: mesos


Description
---

Normally, the mesos-master performs allocation cycles in response
to protocol messages and cluster events. This ties the cost of
allocation to the amount of churn in the environment, which can led
to unpredictable performance. Add an option to disable this, meaning
that allocations are only every performed by the batch interval
task. This makes resource allocation a fixed cost, independent of
the amount of environmental churn.


Diffs (updated)
-

  docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
  src/master/allocator/mesos/allocator.hpp 
aa55755a9c3250579e9366bdbc17a2449e95d659 
  src/master/allocator/mesos/hierarchical.hpp 
38f8fd2c84314bb3731684d0e9795cb4f50a227e 
  src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
  src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/tests/hierarchical_allocator_tests.cpp 
9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
  src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 

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


Testing
---

"make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
weeks.


Thanks,

James Peach

Re: Review Request 37462: Add support for version detection and parsing.

[Ben Mahler]

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


Looks good! Should be shippable after another round.


src/linux/perf.hpp (line 56)


Whoops, doesn't this comment belong on the parse function?



src/linux/perf.cpp (lines 278 - 281)


Hm.. can't we just add this as an overload of supported?

```
bool supported();
bool supported(const Version& version);
```

The first being only in the .cpp file, and let's stick it right below the 
existing supported to make the relationship clear.



src/linux/perf.cpp (lines 324 - 353)


To keep it clean, how about we just do the supported check initially, and 
avoid the 'check' lambda entirely:

```
if (!supported()) {
  return Failure("Perf is not supported");
}

internal::Perf* perf = new internal::Perf(argv);
Future output = perf->output();
spawn(perf, true);

auto parse = [start, duration](
const Version& version,
const string& output) ->
Future> {
  Try> result =
perf::parse(output, version);

  if (result.isError()) {
return Failure("Failed to parse perf sample: " + result.error());
  }

  foreachvalue (mesos::PerfStatistics& statistics, result.get()) {
statistics.set_timestamp(start.secs());
statistics.set_duration(duration.secs());
  }

  return result.get();
};

process::collect(perf::version(), output)
  .then(parse);
```

Note that there are races here either way, where the version we check might 
not match the version of perf that provided the sample.

These lambdas are getting ugly :(



src/linux/perf.cpp (line 391)


Let's just make an overload of supported that takes a version, see my 
comment above.



src/linux/perf.cpp (line 408)


Let's just omit these shas since we can just look at the tags instead:

https://github.com/torvalds/linux/blob/v4.0/tools/perf/perf.c

Ditto below.



src/linux/perf.cpp (lines 410 - 413)


How about just showing the two formats, and above where we call split() 
just saying that we use split because some fields may be empty (e.g. unit, see 
below). Seems a bit odd to show the empty unit cases here as extra cases.



src/linux/perf.cpp (lines 414 - 417)


Braces please :)

Also, this can just be a single statement?

```
if (tokens.size() == 4 || tokens.size() = 6) {
  ...
}
```



src/linux/perf.cpp (lines 422 - 423)


Braces please :)



src/linux/perf.cpp (lines 427 - 428)


Braces please :)



src/linux/perf.cpp (line 430)


The caller will print the line, let's just say what's wrong here (i.e. 
unexpected number of tokens?).

Also let's add a newline above this.



src/linux/perf.cpp (line 443)


Do you have a tool that is removing spaces on if statements, or?



src/tests/containerizer/perf_tests.cpp 


Why did this get removed?



src/tests/containerizer/perf_tests.cpp (line 69)


Please wrap these onto the next line, ditto below.


- Ben Mahler


On Sept. 1, 2015, 8:45 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37462/
> ---
> 
> (Updated Sept. 1, 2015, 8:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for version detection and parsing.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp c5630118f4858b1a805f25a2db7a24ca0989 
>   src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee 
>   src/tests/containerizer/perf_tests.cpp 
> 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 
> 
> Diff: https://reviews.apache.org/r/37462/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> Perf tests may fail on many machines because the tests are currently using 
> the version of perf installed on the machine to choose decode.  The next 
> patch

Re: Review Request 37532: Add QUIESCE call interface to the scheduler

[Marco Massenzio]

> On Aug. 20, 2015, 7:39 p.m., Marco Massenzio wrote:
> > include/mesos/scheduler/scheduler.proto, lines 314-315
> > 
> >
> > This comments does not read well: what is the timeout? also, it would 
> > be good to have a bit of info about what the filters are
> > 
> > (eg, are they 'inclusion' or 'exclusion' filters? etc.)
> 
> Guangya Liu wrote:
> Marco, can you pls show more the difference between "inclusion" and 
> "exclusion" filters? I'm not quite catch this point. Thanks.

Well, I am assuming that the `filters` here will take a conditional action 
based on a `Predicate`? (I am sorry, I don't really know what the `Filters` are 
for here - this was part of what I was asking for more documentation about this 
- please assume that the reader may not have access to all the source code - 
this is certainly true for people using Java bindings, who only usually look at 
the javadoc).

So, if you have method `foo()` that takes some `filters` to take some `action` 
on a given set of objects - the filters are "inclusive" if they will result in 
`action` being taken only on those objects for which `Predicate(Object) == 
true` - "exclusion" filters will instead **not** take action under the same 
circumstance.

Think of it as `-v` in `grep` :)


> On Aug. 20, 2015, 7:39 p.m., Marco Massenzio wrote:
> > src/tests/scheduler_tests.cpp, line 1003
> > 
> >
> > so, this is a good test, but I would really like to see one where we 
> > ask Master to keep quiet for a while and we don't get offers during that 
> > period of time, then we start getting again.
> > 
> > It may require some fiddling around with `Clock`s and all that jazz, 
> > but it would give us more confidence that this whole thing works.
> > 
> > Also - some tests around more complex filtering (if any? maybe this is 
> > there is, then it's fine).
> 
> Guangya Liu wrote:
> I'm planning to add more test cases in another patch, make sense? Thanks.

Ok - so long as the patches are related and will be committed together.
Please make sure to mark them accordingly.


- Marco


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


On Aug. 31, 2015, 1:23 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37532/
> ---
> 
> (Updated Aug. 31, 2015, 1:23 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is just part of MESOS-3037, this patch only add the interface
> of QUIESCE call.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
>   include/mesos/scheduler/scheduler.proto 
> 89daf8a6b74057ee156b3ad691397e76fcb835b8 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
> 
> Diff: https://reviews.apache.org/r/37532/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37466: Update perf tests to including testing the supported perf output formats.

[Ben Mahler]

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


Thanks for testing this!


src/tests/containerizer/perf_tests.cpp (line 67)


This one is never instantiated!



src/tests/containerizer/perf_tests.cpp (line 85)


All of these classes should include Perf in the name, otherwise these are 
not clear in the test results:

ParseCgroups/ParseCgroupsTest.ParseCgroups/0

How about:

Version/PerfCgroupsTest.Parse/0



src/tests/containerizer/perf_tests.cpp (lines 113 - 131)


To be consistent with the other code, please instantiate right after the 
class definition, rather than the test definition. i.e. move this up to right 
below the ParseCgroupsTest class.

Ditto for the others.



src/tests/containerizer/perf_tests.cpp (lines 153 - 164)


I don't think you need the comments here.

Also, shouldn't you do s/4.1.0/4.0.0/ here?

Ditto for the other instantiations.


- Ben Mahler


On Sept. 1, 2015, 10:05 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> ---
> 
> (Updated Sept. 1, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update perf tests to including testing the supported perf output formats.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/perf_tests.cpp 
> 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 
> 
> Diff: https://reviews.apache.org/r/37466/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 37532: Add QUIESCE call interface to the scheduler

[Guangya Liu]

> On 八月 20, 2015, 7:39 p.m., Marco Massenzio wrote:
> > include/mesos/scheduler/scheduler.proto, lines 314-315
> > 
> >
> > This comments does not read well: what is the timeout? also, it would 
> > be good to have a bit of info about what the filters are
> > 
> > (eg, are they 'inclusion' or 'exclusion' filters? etc.)
> 
> Guangya Liu wrote:
> Marco, can you pls show more the difference between "inclusion" and 
> "exclusion" filters? I'm not quite catch this point. Thanks.
> 
> Marco Massenzio wrote:
> Well, I am assuming that the `filters` here will take a conditional 
> action based on a `Predicate`? (I am sorry, I don't really know what the 
> `Filters` are for here - this was part of what I was asking for more 
> documentation about this - please assume that the reader may not have access 
> to all the source code - this is certainly true for people using Java 
> bindings, who only usually look at the javadoc).
> 
> So, if you have method `foo()` that takes some `filters` to take some 
> `action` on a given set of objects - the filters are "inclusive" if they will 
> result in `action` being taken only on those objects for which 
> `Predicate(Object) == true` - "exclusion" filters will instead **not** take 
> action under the same circumstance.
> 
> Think of it as `-v` in `grep` :)

Thanks Marco. Here the filter is just a refused time out, when this time out 
reached, the frameworks can get resource offers again. Quiesce offer will set 
filters to frameworks to disable resource offers and revive offer will clear 
the filter to enable resource offering. The quiesce offer will also be revived 
when the filter time out reached.


- Guangya


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


On 八月 31, 2015, 1:23 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37532/
> ---
> 
> (Updated 八月 31, 2015, 1:23 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is just part of MESOS-3037, this patch only add the interface
> of QUIESCE call.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
>   include/mesos/scheduler/scheduler.proto 
> 89daf8a6b74057ee156b3ad691397e76fcb835b8 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
> 
> Diff: https://reviews.apache.org/r/37532/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37958: Add an option to only perform batch allocations.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 九月 1, 2015, 11:10 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated 九月 1, 2015, 11:10 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37989: Replace hard-coded reap interval with a constant

[Ben Mahler]

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


Hey Guangya, the point of MESOS-1935 was to use these constant functions 
throughout the code base:

```
➜  mesos git:(master) ✗ grep -R 'Seconds(1)' src/tests
src/tests/containerizer/cgroups_tests.cpp:if (elapsed > Seconds(1)) {
src/tests/containerizer/docker_tests.cpp:docker->inspect(containerName, 
Seconds(1));
src/tests/containerizer/docker_tests.cpp:  inspect = 
docker->inspect(containerName, Seconds(1));
src/tests/containerizer/isolator_tests.cpp:  } while (waited < Seconds(1));
src/tests/containerizer/isolator_tests.cpp:  } while (waited < Seconds(1));
src/tests/containerizer/launch_tests.cpp:Clock::advance(Seconds(1));
src/tests/fault_tolerance_tests.cpp:  Clock::advance(Seconds(1)); // 
TODO(benh): Pull out constant from Slave.
src/tests/gc_tests.cpp:  Clock::advance(Seconds(1));
src/tests/gc_tests.cpp:Clock::advance(Seconds(1));
src/tests/health_check_tests.cpp:  Clock::advance(Seconds(1));
src/tests/limiter.hpp:  MockRateLimiter() : process::RateLimiter(1, Seconds(1)) 
{}
src/tests/log_tests.cpp:  EXPECT_GT(Seconds(1), stopwatch.elapsed());
src/tests/log_tests.cpp:  Clock::advance(Seconds(1));
src/tests/log_tests.cpp:  Clock::advance(Seconds(1));
src/tests/master_tests.cpp:  Clock::advance(Seconds(1));
src/tests/rate_limiting_tests.cpp:Clock::advance(Seconds(1));
src/tests/rate_limiting_tests.cpp:Clock::advance(Seconds(1));
src/tests/rate_limiting_tests.cpp:  Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_recovery_tests.cpp:Clock::advance(Seconds(1));
src/tests/slave_tests.cpp:Clock::advance(Seconds(1));
```

Some of these can be replaced by the MAX_REAP_INTERVAL(), for example, this one:
https://github.com/apache/mesos/blob/0.24.0-rc1/src/tests/slave_recovery_tests.cpp#L1687

- Ben Mahler


On Sept. 1, 2015, 5:30 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37989/
> ---
> 
> (Updated Sept. 1, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/reap.hpp 
> ca5acc4c8f5a62a49b7fde83946e283ea40baa65 
>   3rdparty/libprocess/src/reap.cpp 9a994b052c7920a16557c3d880ad499a5efd43cb 
> 
> Diff: https://reviews.apache.org/r/37989/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37958: Add an option to only perform batch allocations.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37958]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 11:10 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37958/
> ---
> 
> (Updated Sept. 1, 2015, 11:10 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the mesos-master performs allocation cycles in response
> to protocol messages and cluster events. This ties the cost of
> allocation to the amount of churn in the environment, which can led
> to unpredictable performance. Add an option to disable this, meaning
> that allocations are only every performed by the batch interval
> task. This makes resource allocation a fixed cost, independent of
> the amount of environmental churn.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf 
>   src/master/flags.cpp 230c1dcedfbbb410b89defd90159d964c001a615 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37958/diff/
> 
> 
> Testing
> ---
> 
> "make check" on CentOS 6 w/ devtoolset-3. Running in production for multiple 
> weeks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37913: Improve allocator filtering by keeping per-slave filter sets.

[Ben Mahler]

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

Ship it!


Looks great, thanks! I'll will get this commmitted for you shortly, left some 
comments for small adjustments I'll make before committing.


src/master/allocator/mesos/hierarchical.hpp (line 216)


You only used this in one location below? In general we've avoided typedefs 
since they require a bit more non-local reasoning at the point in which they 
are being used (i.e. what is a FilterSet? Turns out it's a hashset of Filter 
pointers).



src/master/allocator/mesos/hierarchical.hpp (line 1084)


Can we keep this newline?


- Ben Mahler


On Aug. 28, 2015, 11:46 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37913/
> ---
> 
> (Updated Aug. 28, 2015, 11:46 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3052
> https://issues.apache.org/jira/browse/MESOS-3052
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When frameworks refuse a lot of resources, the list of filters gets
> long. Since the filters are per-slave,
> HierarchicalAllocatorProcess::isFiltered spends a lot of time just
> comparing SlaveID (which tend to be long strings). Eliminate this
> whole problem by organizing the filters by SlaveID in the first
> place.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
> 
> Diff: https://reviews.apache.org/r/37913/diff/
> 
> 
> Testing
> ---
> 
> make check on CentOS 6 w/ devtoolset-3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37024: Exposes mesos version information in components.

[Ben Mahler]

> On Aug. 31, 2015, 10:47 p.m., Ben Mahler wrote:
> > Couple of comments:
> > 
> > Let's not call the actor VersionInfo, since that sounds like a protobuf 
> > message. How about 'VersionProcess' and we avoid the wrapper type entirely 
> > since Version is taken in stout?
> > 
> > Also, can we instantiate this from the main entrypoints rather than inside 
> > master/slave? Would be nice to offer it from the drivers as well (sched.cpp 
> > / exec.cpp).

Could you also follow up with a test for this in a separate patch?


- Ben


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


On Aug. 16, 2015, 5:38 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 16, 2015, 5:38 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor.hpp 72eca97dd84fb1300b37764a3ef3a57fb5e676c2 
>   include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
>   src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/exec/exec.cpp 31e0c2f17a9092d18285828111d27628fb07bc02 
>   src/local/local.cpp 4d98bf23705027f3ba0cbb571289f21b288fe7db 
>   src/master/main.cpp bafc605d6c20bd264b932e44ee80373a3f692734 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
>   src/slave/main.cpp 364dc7fc7ab2e3cef01aea7267dafa014b60e2b9 
>   src/version/version_info.hpp PRE-CREATION 
>   src/version/version_info.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/version 2>/dev/null|jq .
> 
> {
>   "version": "0.24.0",
>   "build_user": "haosdent",
>   "build_time": 1439702338,
>   "build_date": "2015-08-16 13:18:58"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37532: Add QUIESCE call interface to the scheduler

[Marco Massenzio]

> On Aug. 20, 2015, 7:39 p.m., Marco Massenzio wrote:
> > include/mesos/scheduler/scheduler.proto, lines 314-315
> > 
> >
> > This comments does not read well: what is the timeout? also, it would 
> > be good to have a bit of info about what the filters are
> > 
> > (eg, are they 'inclusion' or 'exclusion' filters? etc.)
> 
> Guangya Liu wrote:
> Marco, can you pls show more the difference between "inclusion" and 
> "exclusion" filters? I'm not quite catch this point. Thanks.
> 
> Marco Massenzio wrote:
> Well, I am assuming that the `filters` here will take a conditional 
> action based on a `Predicate`? (I am sorry, I don't really know what the 
> `Filters` are for here - this was part of what I was asking for more 
> documentation about this - please assume that the reader may not have access 
> to all the source code - this is certainly true for people using Java 
> bindings, who only usually look at the javadoc).
> 
> So, if you have method `foo()` that takes some `filters` to take some 
> `action` on a given set of objects - the filters are "inclusive" if they will 
> result in `action` being taken only on those objects for which 
> `Predicate(Object) == true` - "exclusion" filters will instead **not** take 
> action under the same circumstance.
> 
> Think of it as `-v` in `grep` :)
> 
> Guangya Liu wrote:
> Thanks Marco. Here the filter is just a refused time out, when this time 
> out reached, the frameworks can get resource offers again. Quiesce offer will 
> set filters to frameworks to disable resource offers and revive offer will 
> clear the filter to enable resource offering. The quiesce offer will also be 
> revived when the filter time out reached.

Great, thanks - I'd suggest to add this information to the javadoc:
```
// The quiesce message from the framework will disable resource offers until a 
Revive message clears the filter to enable resource offering.
// When the timeout expires, the frameworks will start receiving resource 
offers again.
```

Or something to this effect (I'm sorry, I'm not that familiar with this part 
yet, so I may have gotten the finer details wrong).


- Marco


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


On Aug. 31, 2015, 1:23 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37532/
> ---
> 
> (Updated Aug. 31, 2015, 1:23 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is just part of MESOS-3037, this patch only add the interface
> of QUIESCE call.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
>   include/mesos/scheduler/scheduler.proto 
> 89daf8a6b74057ee156b3ad691397e76fcb835b8 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
> 
> Diff: https://reviews.apache.org/r/37532/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/tests/provisioners/docker_provisioner_tests.cpp, line 232
> > 
> >
> > Why is this needed?

So that we cleanup temporaries created in the SSLTest setup.


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37532: Add QUIESCE call interface to the scheduler

[Guangya Liu]

> On 八月 20, 2015, 7:39 p.m., Marco Massenzio wrote:
> > include/mesos/scheduler/scheduler.proto, lines 314-315
> > 
> >
> > This comments does not read well: what is the timeout? also, it would 
> > be good to have a bit of info about what the filters are
> > 
> > (eg, are they 'inclusion' or 'exclusion' filters? etc.)
> 
> Guangya Liu wrote:
> Marco, can you pls show more the difference between "inclusion" and 
> "exclusion" filters? I'm not quite catch this point. Thanks.
> 
> Marco Massenzio wrote:
> Well, I am assuming that the `filters` here will take a conditional 
> action based on a `Predicate`? (I am sorry, I don't really know what the 
> `Filters` are for here - this was part of what I was asking for more 
> documentation about this - please assume that the reader may not have access 
> to all the source code - this is certainly true for people using Java 
> bindings, who only usually look at the javadoc).
> 
> So, if you have method `foo()` that takes some `filters` to take some 
> `action` on a given set of objects - the filters are "inclusive" if they will 
> result in `action` being taken only on those objects for which 
> `Predicate(Object) == true` - "exclusion" filters will instead **not** take 
> action under the same circumstance.
> 
> Think of it as `-v` in `grep` :)
> 
> Guangya Liu wrote:
> Thanks Marco. Here the filter is just a refused time out, when this time 
> out reached, the frameworks can get resource offers again. Quiesce offer will 
> set filters to frameworks to disable resource offers and revive offer will 
> clear the filter to enable resource offering. The quiesce offer will also be 
> revived when the filter time out reached.
> 
> Marco Massenzio wrote:
> Great, thanks - I'd suggest to add this information to the javadoc:
> ```
> // The quiesce message from the framework will disable resource offers 
> until a Revive message clears the filter to enable resource offering.
> // When the timeout expires, the frameworks will start receiving resource 
> offers again.
> ```
> 
> Or something to this effect (I'm sorry, I'm not that familiar with this 
> part yet, so I may have gotten the finer details wrong).

Sure, will update to the scheduler.proto file. Also update the information a 
bit as following, please show your comments if any. Thanks!
// The quiesce message from the framework will disable resource offers until a 
Revive message clears the filter to enable resource offering or the quiesce 
message filter time out expires


- Guangya


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


On 八月 31, 2015, 1:23 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37532/
> ---
> 
> (Updated 八月 31, 2015, 1:23 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is just part of MESOS-3037, this patch only add the interface
> of QUIESCE call.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
>   include/mesos/scheduler/scheduler.proto 
> 89daf8a6b74057ee156b3ad691397e76fcb835b8 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
> 
> Diff: https://reviews.apache.org/r/37532/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 90
> > 
> >
> > I think "register" is misleading. Allocator is notified that a new 
> > framework joins the cluster and is entitled to participate in resource 
> > sharing.

I was now using adds and re-adds instead in the new patch, comments?


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 109
> > 
> >
> > Let's capiralize `Id` for clarity. Here and everywhere.

Done


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 139-142
> > 
> >
> > This it true for the built-in allocator, but not necessarily for *any* 
> > allocator. Could you please reword putting an accent on *under what 
> > cercumstances* the method is called and maybe what an expected behaviour 
> > may be?

Since the updateFramework is a new API and I can only think up of cases for 
revocable resources related with allocator, can we update this comments in 
future if there are more cases? Thaks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 171
> > 
> >
> > We try to switch to "agent" instead of "slave". Let's do it in the 
> > comments (here and everywhere). Also, let's have a note in the beginning of 
> > the doc saying "agent" is the new "slave".

I see that we already switched to AgentID in v1 API, but the V1 API was not 
finished. Is it possible that we keep slave here and fix this in another RR? Do 
you think that we need to file a new bug or epic to address this? Thanks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 213
> > 
> >
> > I think we remove only outstanding offers, right?

Alex, can you please show mode detail for what is outstanding offers? Thanks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 222
> > 
> >
> > I think "agent" is more clear than "host" here. You maybe refered to 
> > the fact that the whitelist consists of hostnames, but that's slightly 
> > different and should be documented in the whitelist class.

Here I was updating to "host" to "slave" instead and want to address the issue 
of "slave"->"agent" in another patch, make sense?


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 226
> > 
> >
> > I don't think this is correct. AFAIK whitelist contains slaves that 
> > should not participate in the allocation, basically, they are deactivated.

I cite some code from allocator here, it seems that only hosts in whitelist can 
offer resources?

foreach (const SlaveID& slaveId, slaveIds) {
// Don't send offers for non-whitelisted and deactivated slaves.
if (!isWhitelisted(slaveId) || !slaves[slaveId].activated) {
  continue;
}


- Guangya


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


On 九月 1, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 九月 1, 2015, 2:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 2, 2015, 3:06 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


Changes
---

Address Alex's comments. Thanks Alex for the on time and detail review!


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37921: Add Copy backend for provisioners.

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/backends/copy.cpp (lines 105 - 113)


Wow, I guess I hadn't realized what you meant by "making sure the layers 
have the same basename" and I overlooked the fact that when multiple layers are 
applied every layer other than the 1st one is going to have the rootfs dir 
already created...

I think it's too much of a restriction to impose the same basename for all 
rootfes and it's not the backend's position to know that they should all be the 
same.

Given that there is unfortunately no standard way of doing this, I think 
I'll be OK with either of the following two approaches:

1) The following two commands can make sure the source overwrites the 
target event if it is a diretory.

GNU cp:
cp -aT source target // -T makes sure the contents are copied.

OSX cp:
cp a source/ target // The trailing slash makes sure the contents instead 
of the directory is copied.

So we can do something like 

#ifdef __APPLE__
  source += "/"
#else
  options += "T"
#endif

2) Use your previous approach with "/*" expansion but with quotes around 
the paths.

Neither is ideal but is no worse than the system commands we are already 
invoking.

I am OK with either. What do you think?


- Jiang Yan Xu


On Aug. 31, 2015, 5:58 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> ---
> 
> (Updated Aug. 31, 2015, 5:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2968
> https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/backend.cpp 
> 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> d321850613223a2357ca1646a9d988d05171772c 
> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 2, 2015, 3:06 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 2, 2015, 3:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 38040: Refactor docker provisioner store to act as read-through cache.

[Timothy Chen]

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

Review request for mesos, Jie Yu and Jiang Yan Xu.


Repository: mesos


Description
---

Refactor docker provisioner store to act as read-through cache.


Diffs
-

  src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/tests/containerizer/provisioner.hpp 
a26b8138d8cc3086058b15a797dd15354a84019f 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 37921: Add Copy backend for provisioners.

[Timothy Chen]

> On Sept. 2, 2015, 3:07 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/backends/copy.cpp, lines 108-116
> > 
> >
> > Wow, I guess I hadn't realized what you meant by "making sure the 
> > layers have the same basename" and I overlooked the fact that when multiple 
> > layers are applied every layer other than the 1st one is going to have the 
> > rootfs dir already created...
> > 
> > I think it's too much of a restriction to impose the same basename for 
> > all rootfes and it's not the backend's position to know that they should 
> > all be the same.
> > 
> > Given that there is unfortunately no standard way of doing this, I 
> > think I'll be OK with either of the following two approaches:
> > 
> > 1) The following two commands can make sure the source overwrites the 
> > target event if it is a diretory.
> > 
> > GNU cp:
> > cp -aT source target // -T makes sure the contents are copied.
> > 
> > OSX cp:
> > cp a source/ target // The trailing slash makes sure the contents 
> > instead of the directory is copied.
> > 
> > So we can do something like 
> > 
> > #ifdef __APPLE__
> >   source += "/"
> > #else
> >   options += "T"
> > #endif
> > 
> > 2) Use your previous approach with "/*" expansion but with quotes 
> > around the paths.
> > 
> > Neither is ideal but is no worse than the system commands we are 
> > already invoking.
> > 
> > I am OK with either. What do you think?

hmm, I think I'll go for 1) option.


- Timothy


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


On Sept. 1, 2015, 12:58 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> ---
> 
> (Updated Sept. 1, 2015, 12:58 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2968
> https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/backend.cpp 
> 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> d321850613223a2357ca1646a9d988d05171772c 
> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 410
> > 
> >
> > I think ideally we can introduce something in libprocess so we can 
> > stream results to disk with splice or something like that, avoid as much 
> > copies as we can.

yeah would be nice to have such functionality in libproces.


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 306
> > 
> >
> > Put this in a constant.

the constant should ideally be in http namespace but there we use literals.


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37024: Exposes mesos version information in components.

[haosdent huang]

> On Aug. 31, 2015, 10:47 p.m., Ben Mahler wrote:
> > Couple of comments:
> > 
> > Let's not call the actor VersionInfo, since that sounds like a protobuf 
> > message. How about 'VersionProcess' and we avoid the wrapper type entirely 
> > since Version is taken in stout?
> > 
> > Also, can we instantiate this from the main entrypoints rather than inside 
> > master/slave? Would be nice to offer it from the drivers as well (sched.cpp 
> > / exec.cpp).
> 
> Ben Mahler wrote:
> Could you also follow up with a test for this in a separate patch?

Sure


- haosdent


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


On Aug. 16, 2015, 5:38 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 16, 2015, 5:38 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor.hpp 72eca97dd84fb1300b37764a3ef3a57fb5e676c2 
>   include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
>   src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/exec/exec.cpp 31e0c2f17a9092d18285828111d27628fb07bc02 
>   src/local/local.cpp 4d98bf23705027f3ba0cbb571289f21b288fe7db 
>   src/master/main.cpp bafc605d6c20bd264b932e44ee80373a3f692734 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
>   src/slave/main.cpp 364dc7fc7ab2e3cef01aea7267dafa014b60e2b9 
>   src/version/version_info.hpp PRE-CREATION 
>   src/version/version_info.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/version 2>/dev/null|jq .
> 
> {
>   "version": "0.24.0",
>   "build_user": "haosdent",
>   "build_time": 1439702338,
>   "build_date": "2015-08-16 13:18:58"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37622: Maintenance Primitives: Shutdown & remove slave when maintenance is started.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 八月 26, 2015, 2:13 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37622/
> ---
> 
> (Updated 八月 26, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
>   src/tests/master_maintenance_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37622/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>