Re: Review Request 41854: Added module initialization to Master main().

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On Feb. 13, 2016, 12:32 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41854/
> ---
> 
> (Updated Feb. 13, 2016, 12:32 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kapil Arya.
> 
> 
> Bugs: MESOS-4253
> https://issues.apache.org/jira/browse/MESOS-4253
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added module initialization to Master main().
> 
> 
> Diffs
> -
> 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/slave/main.cpp 222198ca89f672332cb80773a3f36fe1f0438f64 
> 
> Diff: https://reviews.apache.org/r/41854/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> [==] 932 tests from 119 test cases ran. (210502 ms total)
> [  PASSED  ] 932 tests.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 41760: Add initialization method to Anonymous class.

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On Feb. 13, 2016, 1:24 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41760/
> ---
> 
> (Updated Feb. 13, 2016, 1:24 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kapil Arya.
> 
> 
> Bugs: MESOS-4253
> https://issues.apache.org/jira/browse/MESOS-4253
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-4253
> 
> To provide a basic "context" to anonymous modules, we provide
> an `initialize(const FlagsBase&)` method that will be invoked
> shortly after creation of the module class.
> 
> 
> Diffs
> -
> 
>   include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b 
>   src/examples/test_anonymous_module.hpp 
> 3da33a42f04b7421cee8fbb85e29b66e352f5894 
>   src/examples/test_anonymous_module.cpp 
> dd291cff3b5d47337e371cd2c1082fd6716af3fc 
>   src/slave/main.cpp 222198ca89f672332cb80773a3f36fe1f0438f64 
>   src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 
>   src/tests/module.hpp 4b32f29f2ce76100433621a5cb6b8cc87c9b38f8 
>   src/tests/module.cpp 8cc305c0ef606b07eea39d548d3165a2bb2b042a 
> 
> Diff: https://reviews.apache.org/r/41760/diff/
> 
> 
> Testing
> ---
> 
> Unit tests pass.
> 
> Also implemented in the 
> [execute-module](http://github.com/massenz/execute-module) - and it works 
> there too:
> ```
> I0102 17:38:26.180788 19971 main.cpp:272] Initializing anonymous module 
> 'com_alertavert_mesos_RemoteExecution'
> I0102 17:38:26.180800 19971 main.cpp:278] Sending flags to module 
> 'com_alertavert_mesos_RemoteExecution'
> I0102 17:38:26.180810 19971 execute_module.hpp:129] Executing initialize() 
> for module; parsing runtime flags
> I0102 17:38:26.181658 19971 execute_module.hpp:139] Configured work dir to 
> [/tmp/agent] and Sandbox dir to [/mnt/mesos/sandbox]
> ```
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 44112: Updated tests to use /state endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44110, 44111, 44112]

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

- Mesos ReviewBot


On Feb. 27, 2016, 2:18 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44112/
> ---
> 
> (Updated Feb. 27, 2016, 2:18 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Vinod Kone.
> 
> 
> Bugs: MESOS-4509
> https://issues.apache.org/jira/browse/MESOS-4509
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated tests to use /state endpoint instead of deprecated state.json 
> endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 982468f851cd9d95eb6cde7c57f2d737d46a827c 
>   src/tests/master_tests.cpp 0bd8c0e42f335cad7ed858c6af5aa4f07bb37dbf 
>   src/tests/slave_tests.cpp 322f3ddaf11885d7e61e0e9232c0342e97d8bfa1 
> 
> Diff: https://reviews.apache.org/r/44112/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 43847: Added master/scheduler failover tests for scheduler library.

[Anand Mazumdar]

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

(Updated Feb. 27, 2016, 4:48 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

See summary. This are a follow up to the patches for MESOS-3570 around 
modifying the scheduler library to use HTTP Pipelining.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp 37f17094b3f11fd02468bf51b51b8e65ccb350a9 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 43846: Added support for specifying detector to the callback interface.

[Anand Mazumdar]

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

(Updated Feb. 27, 2016, 4:48 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

This change adds support for passing in a master detector object to the 
callback interface. This can be used for testing master failover scenarios 
similar to how the driver already does it.


Diffs (updated)
-

  include/mesos/v1/scheduler.hpp 5e462c13ec2715888247eb7b4dc7a2b1d53e6bb1 
  src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
  src/tests/mesos.hpp 242a11658c0a9ba4caced9b2b2bdbcb921f7fdd0 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 43846: Added support for specifying detector to the callback interface.

[Anand Mazumdar]

> On Feb. 27, 2016, 2:43 a.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, lines 637-638
> > 
> >
> > Yikes. Why are you doing it like this, instead of passing detector as 
> > an optional argument to the MesosProcess?

Agreed, my bad. I had tried to follow the existing pattern used in 
`TestMesosSchedulerDriver` but should have avoided it.


- Anand


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


On Feb. 22, 2016, 8:24 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43846/
> ---
> 
> (Updated Feb. 22, 2016, 8:24 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4686
> https://issues.apache.org/jira/browse/MESOS-4686
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds support for passing in a master detector object to the 
> callback interface. This can be used for testing master failover scenarios 
> similar to how the driver already does it.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/scheduler.hpp 5e462c13ec2715888247eb7b4dc7a2b1d53e6bb1 
>   src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
>   src/tests/mesos.hpp 242a11658c0a9ba4caced9b2b2bdbcb921f7fdd0 
> 
> Diff: https://reviews.apache.org/r/43846/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43667: Modified existing scheduler tests as an aftermath of pipelining change.

[Anand Mazumdar]

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

(Updated Feb. 27, 2016, 4:45 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Merged r43664 with this review.


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


Repository: mesos


Description
---

This change modifies the existing tests to handle the new 
`connected/disconnected` semantics that were introduced as a result of the 
pipelining change.

Now `connected` is possible to invoked more then once i.e. during test 
shutdown, the master closed the event stream making us call the `disconnected` 
callback. However, the master detector can still detect the same master and 
establish a connection to it while it is shutting down leading to `connected` 
callback be invoked again in some cases.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
b98eedec388813ee795dd83ccc5ff27338209475 
  src/tests/scheduler_tests.cpp 37f17094b3f11fd02468bf51b51b8e65ccb350a9 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 43664: Modified SchedulerTest.Subscribe to not test for failover.

[Anand Mazumdar]

> On Feb. 27, 2016, 2:08 a.m., Vinod Kone wrote:
> > src/tests/scheduler_tests.cpp, line 136
> > 
> >
> > why will there be future invocations?
> 
> Vinod Kone wrote:
> I see the reasoning in the next review. Maybe move this particular change 
> to that review?

Sounds good, discarding this review and merging it with r43667


- Anand


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


On Feb. 17, 2016, 10:45 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43664/
> ---
> 
> (Updated Feb. 17, 2016, 10:45 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
> https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modifies the existing `SchedulerTest.Subscribe` to just test if 
> we are able to subscribe with the master instead of also testing for 
> scheduler failover. 
> 
> Added a `TODO` to add a separate test for testing scheduler failover once 
> MESOS-3339 is resolved. The reasoning for doing so was that the callback 
> interface would simplify writing the test.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp 37f17094b3f11fd02468bf51b51b8e65ccb350a9 
> 
> Diff: https://reviews.apache.org/r/43664/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43662: Added support for pipelining calls to the scheduler library.

[Anand Mazumdar]

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

(Updated Feb. 27, 2016, 4:43 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

Previously, the scheduler library used to chain calls on previous call 
responses. This was inherently slow. This change adds support for pipelining 
all calls to the master on a single connection via the `http::Connection` 
abstraction in libprocess.

This change also adds support for handling various error scenarios when we 
notice a disconnection instead of just relying on the master detector for 
invoking the `disconnected` callback.


Diffs (updated)
-

  src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 44113: Cleaned up assertions in test cases.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43635, 44126, 43636, 44113]

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

- Mesos ReviewBot


On Feb. 27, 2016, 1:56 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44113/
> ---
> 
> (Updated Feb. 27, 2016, 1:56 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that `EXPECT_EQ` and friends use "expected, actual" as the
> order of arguments.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_spec_tests.cpp 
> 5799dc9c871c6ccaddb209ab7ec1112b192f3d41 
>   src/tests/containerizer/docker_tests.cpp 
> 620819330847a10d9dcd817968df9d2b180a9a29 
>   src/tests/containerizer/fs_tests.cpp 
> 29e43877612fa151e6f6d79268a7411272a7bfeb 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 25b28ef8fa5aae81e8dd0c9e33df4160dd912ce8 
>   src/tests/module_tests.cpp 121d65337bdf29f6049ac44bfd903a1f5ea1a09d 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/tests/sorter_tests.cpp 22bc618f9b4958fbd8e6c5dfee94b26fe13ec47a 
> 
> Diff: https://reviews.apache.org/r/44113/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44129: Fixed variable shadowing in HookManager::slavePreLaunchDockerHook.

[Kapil Arya]

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

(Updated Feb. 26, 2016, 11:10 p.m.)


Review request for mesos, haosdent huang, Joris Van Remoortere, Kevin Devroede, 
and Michael Park.


Changes
---

Fixed one last variable name update.


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


Repository: mesos


Description (updated)
---

Fixed variable shadowing in HookManager::slavePreLaunchDockerHook.


Diffs (updated)
-

  src/hook/manager.cpp 704b166725c0b356f2acb9bb1539a72cefd9b7ae 

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 44058: Add metrics for {RESERVE, UNRESERVE} and {CREATE, DESTROY} offer operation

[Guangya Liu]

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



The document monitoring.md and some test cases are also needed.

- Guangya Liu


On 二月 26, 2016, 7:19 a.m., fan du wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44058/
> ---
> 
> (Updated 二月 26, 2016, 7:19 a.m.)
> 
> 
> Review request for mesos, Greg Mann, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4492
> https://issues.apache.org/jira/browse/MESOS-4492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add metrics for {RESERVE, UNRESERVE} and {CREATE, DESTROY} offer operation
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 
>   src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 
> 
> Diff: https://reviews.apache.org/r/44058/diff/
> 
> 
> Testing
> ---
> 
> 1. make check on Centos-7 (3.10.0-123.el7.x86_640
> 2. Verify its functionality with 'reserve' http endpoint as an test case
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 0.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> # curl -i -d slaveId=6250553a-2f39-4a92-9073-4618d130f433-S1  -d resources='[ 
> { "name": "cpus", "type": "SCALAR","scalar": { "value": 1 
> },"reservation":{"principal": "XiaoHaHa"}}  ]' -X POST  
> ipdc02-kvm-guest2:5050/master/reserve
> HTTP/1.1 200 OK
> Date: Fri, 26 Feb 2016 19:59:01 GMT
> Content-Length: 0
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot  | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 1.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> Thanks,
> 
> fan du
> 
>

Re: Review Request 43969: Added test for Appc image fetcher.

[Jojy Varghese]

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

(Updated Feb. 27, 2016, 4:05 a.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

Added simple appc Fetcher test with mock HTTP image server.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43969: Added test for Appc image fetcher.

[Jojy Varghese]

> On Feb. 27, 2016, 12:21 a.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_appc_tests.cpp, line 539
> > 
> >
> > We might want to serve multiple images. Can you make it more general so 
> > that we can serve any files under 'server' directory?

I thought this is a good start and we can expand as we go. We could add a map 
of `name->path` in the test fixture. I can add a TODO.


- Jojy


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


On Feb. 24, 2016, 11:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43969/
> ---
> 
> (Updated Feb. 24, 2016, 11:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4748
> https://issues.apache.org/jira/browse/MESOS-4748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added simple appc Fetcher test with mock HTTP image server.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 
> 
> Diff: https://reviews.apache.org/r/43969/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 44129: Fixed variable shadowing in HookManager::slavePreLaunchDockerHook.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On Feb. 27, 2016, 3:42 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44129/
> ---
> 
> (Updated Feb. 27, 2016, 3:42 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Joris Van Remoortere, Kevin 
> Devroede, and Michael Park.
> 
> 
> Bugs: MESOS-4693
> https://issues.apache.org/jira/browse/MESOS-4693
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/hook/manager.cpp 704b166725c0b356f2acb9bb1539a72cefd9b7ae 
> 
> Diff: https://reviews.apache.org/r/44129/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 43883: Added allocator metrics for number of offer filters per framework.

[Guangya Liu]

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




src/tests/hierarchical_allocator_tests.cpp (line 2614)


4 spaces

and ditto for the following



src/tests/hierarchical_allocator_tests.cpp (line 2641)


what about move this comments above 
`Clock::advance(flags.allocation_interval);
` and update it to:

// OfferFilter refuse1 will be expired after allocation_interval.

ditto for the following


- Guangya Liu


On 二月 26, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43883/
> ---
> 
> (Updated 二月 26, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4722
> https://issues.apache.org/jira/browse/MESOS-4722
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for number of offer filters per framework.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43883/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 44129: Fixed variable shadowing in HookManager::slavePreLaunchDockerHook.

[Kapil Arya]

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

Review request for mesos, haosdent huang, Joris Van Remoortere, Kevin Devroede, 
and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/hook/manager.cpp 704b166725c0b356f2acb9bb1539a72cefd9b7ae 

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 43999: Use relative path to create libraries symbolic link.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43999]

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

- Mesos ReviewBot


On Feb. 27, 2016, 1:55 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43999/
> ---
> 
> (Updated Feb. 27, 2016, 1:55 a.m.)
> 
> 
> Review request for mesos, James Peach and Kapil Arya.
> 
> 
> Bugs: MESOS-4774
> https://issues.apache.org/jira/browse/MESOS-4774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use relative path to create libraries symbolic link.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5 
> 
> Diff: https://reviews.apache.org/r/43999/diff/
> 
> 
> Testing
> ---
> 
> After make install, the library links will be relative path.
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 43847: Added master/scheduler failover tests for scheduler library.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/scheduler_tests.cpp (line 317)


s/Fail over/Failover/


- Vinod Kone


On Feb. 22, 2016, 8:20 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43847/
> ---
> 
> (Updated Feb. 22, 2016, 8:20 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4686
> https://issues.apache.org/jira/browse/MESOS-4686
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary. This are a follow up to the patches for MESOS-3570 around 
> modifying the scheduler library to use HTTP Pipelining.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp 37f17094b3f11fd02468bf51b51b8e65ccb350a9 
> 
> Diff: https://reviews.apache.org/r/43847/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43846: Added support for specifying detector to the callback interface.

[Vinod Kone]

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




include/mesos/v1/scheduler.hpp (line 77)


I guess you made this protected because it is only used by TestMesos in 
tests? Please add a comment saying as much for posterity.



src/scheduler/scheduler.cpp (line 562)


don't do this. see below.



src/scheduler/scheduler.cpp (lines 637 - 638)


Yikes. Why are you doing it like this, instead of passing detector as an 
optional argument to the MesosProcess?



src/tests/mesos.hpp (lines 968 - 972)


why a new constructor instead of updating the above constructor. you can 
make detector Option> with default None.


- Vinod Kone


On Feb. 22, 2016, 8:24 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43846/
> ---
> 
> (Updated Feb. 22, 2016, 8:24 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4686
> https://issues.apache.org/jira/browse/MESOS-4686
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds support for passing in a master detector object to the 
> callback interface. This can be used for testing master failover scenarios 
> similar to how the driver already does it.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/scheduler.hpp 5e462c13ec2715888247eb7b4dc7a2b1d53e6bb1 
>   src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
>   src/tests/mesos.hpp 242a11658c0a9ba4caced9b2b2bdbcb921f7fdd0 
> 
> Diff: https://reviews.apache.org/r/43846/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 39372: Introduced a callback interface for testing the scheduler library.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 22, 2016, 8:19 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39372/
> ---
> 
> (Updated Feb. 22, 2016, 8:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3339
> https://issues.apache.org/jira/browse/MESOS-3339
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a new callback based testing interface for the scheduler 
> library similar to the already existing one for the executor library.
> 
> This gets rid of the following hack:
> ```cpp
> 
> // Enqueues all received events into a libprocess queue.
> ACTION_P(Enqueue, queue)
> {
>   std::queue events = arg0;
>   while (!events.empty()) {
> // Note that we currently drop HEARTBEATs because most of these tests
> // are not designed to deal with heartbeats.
> // TODO(vinod): Implement DROP_HTTP_CALLS that can filter heartbeats.
> if (events.front().type() == Event::HEARTBEAT) {
>   VLOG(1) << "Ignoring HEARTBEAT event";
> } else {
>   queue->put(events.front());
> }
> events.pop();
>   }
> }
> 
> ```
> 
> New way ( similar to what we do for the driver implementation )
> ```cpp
> 
> EXPECT_CALL(callbacks, heartbeat())
> .WillRepeatedly(Return()); // Ignore heartbeats.
> 
> ```
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 242a11658c0a9ba4caced9b2b2bdbcb921f7fdd0 
> 
> Diff: https://reviews.apache.org/r/39372/diff/
> 
> 
> Testing
> ---
> 
> make check (Would modify the tests to use the new interface in a subsequent 
> change)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 44112: Updated tests to use /state endpoint.

[Joerg Schad]

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

(Updated Feb. 27, 2016, 2:18 a.m.)


Review request for mesos, Alexander Rojas and Vinod Kone.


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


Repository: mesos


Description
---

Updated tests to use /state endpoint instead of deprecated state.json endpoint.


Diffs
-

  src/tests/fault_tolerance_tests.cpp 982468f851cd9d95eb6cde7c57f2d737d46a827c 
  src/tests/master_tests.cpp 0bd8c0e42f335cad7ed858c6af5aa4f07bb37dbf 
  src/tests/slave_tests.cpp 322f3ddaf11885d7e61e0e9232c0342e97d8bfa1 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 44111: Updated UI controller to use master/state endpoint.

[Joerg Schad]

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

(Updated Feb. 27, 2016, 2:17 a.m.)


Review request for mesos, Alexander Rojas, Philip Norman, and Vinod Kone.


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


Repository: mesos


Description
---

Updated UI controller to use master/state endpoint instead of depracted 
state.json endpoint.


Diffs
-

  src/webui/master/static/js/controllers.js 
56a44fccdc2a004463576c710f9eb117646514a2 

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


Testing
---

make check + manually checked the UI


Thanks,

Joerg Schad

Re: Review Request 43664: Modified SchedulerTest.Subscribe to not test for failover.

[Vinod Kone]

> On Feb. 27, 2016, 2:08 a.m., Vinod Kone wrote:
> > src/tests/scheduler_tests.cpp, line 136
> > 
> >
> > why will there be future invocations?

I see the reasoning in the next review. Maybe move this particular change to 
that review?


- Vinod


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


On Feb. 17, 2016, 10:45 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43664/
> ---
> 
> (Updated Feb. 17, 2016, 10:45 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
> https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modifies the existing `SchedulerTest.Subscribe` to just test if 
> we are able to subscribe with the master instead of also testing for 
> scheduler failover. 
> 
> Added a `TODO` to add a separate test for testing scheduler failover once 
> MESOS-3339 is resolved. The reasoning for doing so was that the callback 
> interface would simplify writing the test.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp 37f17094b3f11fd02468bf51b51b8e65ccb350a9 
> 
> Diff: https://reviews.apache.org/r/43664/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43667: Modified existing scheduler tests as an aftermath of pipelining change.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 17, 2016, 10:46 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43667/
> ---
> 
> (Updated Feb. 17, 2016, 10:46 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
> https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modifies the existing tests to handle the new 
> `connected/disconnected` semantics that were introduced as a result of the 
> pipelining change.
> 
> Now `connected` is possible to invoked more then once i.e. during test 
> shutdown, the master closed the event stream making us call the 
> `disconnected` callback. However, the master detector can still detect the 
> same master and establish a connection to it while it is shutting down 
> leading to `connected` callback be invoked again in some cases.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> b98eedec388813ee795dd83ccc5ff27338209475 
>   src/tests/scheduler_tests.cpp 37f17094b3f11fd02468bf51b51b8e65ccb350a9 
> 
> Diff: https://reviews.apache.org/r/43667/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43662: Added support for pipelining calls to the scheduler library.

[Vinod Kone]

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


Fix it, then Ship it!





src/scheduler/scheduler.cpp (line 276)


Can you expand more here. Need to say that we make a copy here because 
'master' and 'connectionId' values might change by the time the 2nd 
`http::connect()` or `connected()` gets called.



src/scheduler/scheduler.cpp (line 282)


s/master.get()/master_/



src/scheduler/scheduler.cpp (line 302)


"Ignoring stale connection attempt"

This can happen even without master failover.



src/scheduler/scheduler.cpp (line 358)


ditto. VLOG(1) ?



src/scheduler/scheduler.cpp (line 392)


I think this will read better as

if (state == CONNECTED || state == SUBSCRIBING || state == SUBSCRIBED)


- Vinod Kone


On Feb. 27, 2016, 1:22 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43662/
> ---
> 
> (Updated Feb. 27, 2016, 1:22 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
> https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the scheduler library used to chain calls on previous call 
> responses. This was inherently slow. This change adds support for pipelining 
> all calls to the master on a single connection via the `http::Connection` 
> abstraction in libprocess.
> 
> This change also adds support for handling various error scenarios when we 
> notice a disconnection instead of just relying on the master detector for 
> invoking the `disconnected` callback.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
> 
> Diff: https://reviews.apache.org/r/43662/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43636: Cleaned up various code in a test file.

[Neil Conway]

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

(Updated Feb. 27, 2016, 1:58 a.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Repository: mesos


Description
---

Cleaned up various code in a test file.


Diffs (updated)
-

  src/tests/teardown_tests.cpp 5753559003d703138d2bbee6a1ac93473ba0b0c0 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Neil Conway]

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

(Updated Feb. 27, 2016, 1:57 a.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

Scalar resource values are represented using floating point. As a result, users
could see unexpected results when accepting offers and making reservations for
fractional resources: values like "0.1" cannot be precisely represented using
standard floating point, and the resource values returned to frameworks might
contain an unpredictable amount of roundoff error.

This commit adjusts the master to use fixed-point when doing internal
computations on scalar resource values. The fixed-point format only supports
three decimal digits of precision: that is, fractional resource values like
"0.001" will be supported, but "0.0001" will not be.


Diffs (updated)
-

  docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
  docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 
  include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 
  include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 
  src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 
  src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
  src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 
  src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 

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


Testing
---

make check

Manually verified that some of the floating point oddities in 
https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
is applied, although I wasn't able to reproduce the crash described in that 
ticket.

REVIEW NOTES:

* We don't currently emit a warning when discarding additional digits of 
precision from input scalar resource values. Should we? That would require 
identifying all the points where a resource value is seemed to be 
"user-provided", and also runs the risk of generating a ton of log messages 
when an old framework is used.
* Similarly, if the user gives us a resource value and we don't do anything to 
it, we won't discard any additional precision that appears in the value -- the 
precision only gets discarded when we apply an operator like `+` or `-`. 
Unclear if we should trim additional precision from all scalar resource values 
more aggressively.

PERFORMANCE:

This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
applied (optimized build on my laptop):

```
[ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
Using 2000 slaves and 200 frameworks
round 0 allocate took 2.192425secs to make 200 offers
round 1 allocate took 2.221243secs to make 200 offers
round 2 allocate took 2.236314secs to make 200 offers
round 3 allocate took 2.224045secs to make 200 offers
round 4 allocate took 2.232822secs to make 200 offers
round 5 allocate took 2.264807secs to make 200 offers
round 6 allocate took 2.224853secs to make 200 offers
round 7 allocate took 2.224829secs to make 200 offers
round 8 allocate took 2.24862secs to make 200 offers
round 9 allocate took 2.2556secs to make 200 offers
round 10 allocate took 2.256616secs to make 200 offers
```

And after applying this RR:

```
[ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
Using 2000 slaves and 200 frameworks
round 0 allocate took 2.267919secs to make 200 offers
round 1 allocate took 2.202843secs to make 200 offers
round 2 allocate took 2.20426secs to make 200 offers
round 3 allocate took 2.263887secs to make 200 offers
round 4 allocate took 2.266237secs to make 200 offers
round 5 allocate took 2.276957secs to make 200 offers
round 6 allocate took 2.291821secs to make 200 offers
round 7 allocate took 2.261839secs to make 200 offers
round 8 allocate took 2.325696secs to make 200 offers
round 9 allocate took 2.310469secs to make 200 offers
round 10 allocate took 2.21802secs to make 200 offers
```

Which suggests to me that any performance hit is pretty minimal.


Thanks,

Neil Conway

Review Request 44126: Fixed a few style issues in the docs.

[Neil Conway]

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

Review request for mesos, Joris Van Remoortere and Michael Park.


Repository: mesos


Description
---

Fixed a few style issues in the docs.


Diffs
-

  docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 44113: Cleaned up assertions in test cases.

[Neil Conway]

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

(Updated Feb. 27, 2016, 1:56 a.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Rebase.


Repository: mesos


Description
---

Ensure that `EXPECT_EQ` and friends use "expected, actual" as the
order of arguments.


Diffs (updated)
-

  src/tests/containerizer/docker_spec_tests.cpp 
5799dc9c871c6ccaddb209ab7ec1112b192f3d41 
  src/tests/containerizer/docker_tests.cpp 
620819330847a10d9dcd817968df9d2b180a9a29 
  src/tests/containerizer/fs_tests.cpp 29e43877612fa151e6f6d79268a7411272a7bfeb 
  src/tests/containerizer/provisioner_backend_tests.cpp 
25b28ef8fa5aae81e8dd0c9e33df4160dd912ce8 
  src/tests/module_tests.cpp 121d65337bdf29f6049ac44bfd903a1f5ea1a09d 
  src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
  src/tests/sorter_tests.cpp 22bc618f9b4958fbd8e6c5dfee94b26fe13ec47a 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43999: Use relative path to create libraries symbolic link.

[Zhiwei Chen]

> On Feb. 27, 2016, 1:36 a.m., Kapil Arya wrote:
> > src/Makefile.am, lines 2010-2011
> > 
> >
> > Minor nit. Can we fix the alignment to four spaces to be consistent 
> > with the rest of the block? Also align the trailing slashes? (It's possible 
> > that they are already aligned, just not showing up on the RB diff.

Thanks, it was aligned in git diff.


- Zhiwei


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


On Feb. 26, 2016, 4:18 p.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43999/
> ---
> 
> (Updated Feb. 26, 2016, 4:18 p.m.)
> 
> 
> Review request for mesos, James Peach and Kapil Arya.
> 
> 
> Bugs: MESOS-4774
> https://issues.apache.org/jira/browse/MESOS-4774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use relative path to create libraries symbolic link.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5 
> 
> Diff: https://reviews.apache.org/r/43999/diff/
> 
> 
> Testing
> ---
> 
> After make install, the library links will be relative path.
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 43999: Use relative path to create libraries symbolic link.

[Zhiwei Chen]

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

(Updated Feb. 27, 2016, 9:55 a.m.)


Review request for mesos, James Peach and Kapil Arya.


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


Repository: mesos


Description
---

Use relative path to create libraries symbolic link.


Diffs (updated)
-

  src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5 

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


Testing
---

After make install, the library links will be relative path.


Thanks,

Zhiwei Chen

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Michael Park]

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


Fix it, then Ship it!





src/common/values.cpp (lines 96 - 100)


Can we just leverage the `operator+=`?

```
Value::Scalar result = left;
result += right;
return result;
```

Same for `operator-` and the `v1` API.


- Michael Park


On Feb. 27, 2016, 12:10 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43635/
> ---
> 
> (Updated Feb. 27, 2016, 12:10 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-4687
> https://issues.apache.org/jira/browse/MESOS-4687
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Scalar resource values are represented using floating point. As a result, 
> users
> could see unexpected results when accepting offers and making reservations for
> fractional resources: values like "0.1" cannot be precisely represented using
> standard floating point, and the resource values returned to frameworks might
> contain an unpredictable amount of roundoff error.
> 
> This commit adjusts the master to use fixed-point when doing internal
> computations on scalar resource values. The fixed-point format only supports
> three decimal digits of precision: that is, fractional resource values like
> "0.001" will be supported, but "0.0001" will not be.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
>   docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 
>   include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 
>   include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 
>   src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 
>   src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 
> 
> Diff: https://reviews.apache.org/r/43635/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manually verified that some of the floating point oddities in 
> https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
> is applied, although I wasn't able to reproduce the crash described in that 
> ticket.
> 
> REVIEW NOTES:
> 
> * We don't currently emit a warning when discarding additional digits of 
> precision from input scalar resource values. Should we? That would require 
> identifying all the points where a resource value is seemed to be 
> "user-provided", and also runs the risk of generating a ton of log messages 
> when an old framework is used.
> * Similarly, if the user gives us a resource value and we don't do anything 
> to it, we won't discard any additional precision that appears in the value -- 
> the precision only gets discarded when we apply an operator like `+` or `-`. 
> Unclear if we should trim additional precision from all scalar resource 
> values more aggressively.
> 
> PERFORMANCE:
> 
> This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
> applied (optimized build on my laptop):
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.192425secs to make 200 offers
> round 1 allocate took 2.221243secs to make 200 offers
> round 2 allocate took 2.236314secs to make 200 offers
> round 3 allocate took 2.224045secs to make 200 offers
> round 4 allocate took 2.232822secs to make 200 offers
> round 5 allocate took 2.264807secs to make 200 offers
> round 6 allocate took 2.224853secs to make 200 offers
> round 7 allocate took 2.224829secs to make 200 offers
> round 8 allocate took 2.24862secs to make 200 offers
> round 9 allocate took 2.2556secs to make 200 offers
> round 10 allocate took 2.256616secs to make 200 offers
> ```
> 
> And after applying this RR:
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.267919secs to make 200 offers
> round 1 allocate took 2.202843secs to make 200 offers
> round 2 allocate took 2.20426secs to make 200 offers
> round 3 allocate took 2.263887secs to make 200 offers
> round 4 allocate took 2.266237secs to make 200 offers
> round 5 allocate took 2.276957secs to make 200 offers
> round 6 allocate took 2.291821secs to make 200 offers
> round 7 allocate took 2.261839secs to make 200 offers

Re: Review Request 43662: Added support for pipelining calls to the scheduler library.

[Anand Mazumdar]

> On Feb. 26, 2016, 8:29 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 394
> > 
> >
> > what if we are in CONNECTING state?

Good catch. I had missed that. Added a explicit check for CONNECTING too.


> On Feb. 26, 2016, 8:29 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 585
> > 
> >
> > what about other states?

Added an explicit check for SUBSCRIBED.


> On Feb. 26, 2016, 8:29 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 549
> > 
> >
> > Do we need 'subscribed' as a member variable now? Can we not detect 
> > this via state and/or connectionID?

As per our discussion, let me do this change in a separate patch both for 
scheduler/executor.


- Anand


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


On Feb. 25, 2016, 4:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43662/
> ---
> 
> (Updated Feb. 25, 2016, 4:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
> https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the scheduler library used to chain calls on previous call 
> responses. This was inherently slow. This change adds support for pipelining 
> all calls to the master on a single connection via the `http::Connection` 
> abstraction in libprocess.
> 
> This change also adds support for handling various error scenarios when we 
> notice a disconnection instead of just relying on the master detector for 
> invoking the `disconnected` callback.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
> 
> Diff: https://reviews.apache.org/r/43662/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43662: Added support for pipelining calls to the scheduler library.

[Anand Mazumdar]

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

(Updated Feb. 27, 2016, 1:22 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

Previously, the scheduler library used to chain calls on previous call 
responses. This was inherently slow. This change adds support for pipelining 
all calls to the master on a single connection via the `http::Connection` 
abstraction in libprocess.

This change also adds support for handling various error scenarios when we 
notice a disconnection instead of just relying on the master detector for 
invoking the `disconnected` callback.


Diffs (updated)
-

  src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 44108: Added 'Updating the wiki' step to the release guide.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44108]

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

- Mesos ReviewBot


On Feb. 26, 2016, 11:34 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44108/
> ---
> 
> (Updated Feb. 26, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the recent Mesos Community Sync, it was reminded that we should be keeping 
> the [Mesos Release 
> Planning](https://cwiki.apache.org/confluence/display/MESOS/Mesos+Release+Planning)
>  wiki page up to date.
> This patch adds this step to the release guide so that we don't forget.
> 
> 
> Diffs
> -
> 
>   docs/release-guide.md f0963e750494fcf1f9d80e87bac8dbd6f76adc9f 
> 
> Diff: https://reviews.apache.org/r/44108/diff/
> 
> 
> Testing
> ---
> 
> `rake && rake dev`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 44029: Fetcher::basename should ignore query strings and fragments.

[Jiang Yan Xu]

> On Feb. 26, 2016, 5:40 a.m., Bernd Mathiske wrote:
> > There is a more elaborate solution to this problem 
> > (https://reviews.apache.org/r/40054), but it requires a lot of code to 
> > implement URL parsing. Until we finalize that, I think the patch at hand 
> > gets the most urgent job done.
> 
> Jiang Yan Xu wrote:
> The concern here is that it changes the current behavior of the fetcher 
> output. Although we agree that probably most people would find the query 
> string in the filename annoying, there could be people who rely on this (and 
> with a workaround in place) and this will suddenly break things.
> 
> The explicit output filename proposed in 
> https://issues.apache.org/jira/browse/MESOS-3367 sounds like the right 
> approach.
> 
> Jiang Yan Xu wrote:
> When we have an alternative approach available to users, it's easier to 
> then come back and impose a deprecate cycle for the default behavior. What do 
> you think?
> 
> James Peach wrote:
> Yes, there's definitely compatibility concern (for example, 
> ``http://foo/bar/baz?filename.zip`` would no longer work. After chatting with 
> Yan, maybe it is sufficient to ignore the extension and just try all the 
> extractors. If it is extractable one of them will succeed. I don't think 
> there's any compatibility concerns here and we wouldn't need to do a 
> deprecation cycle.

So it occurred to me that here's an edge case:

Some executable archieves are in fact zip files, e.g., jar and pex files. In 
the past they wouldn't get automatically decompressed due to their suffixes and 
not that unzip couldn't decomopress them.

The original file is still going to be there but this implicates disk quota, 
etc. Seemingly not an extremely dangerous case but probably still warrants a 
deprecation cycle.


- Jiang Yan


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


On Feb. 25, 2016, 10:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44029/
> ---
> 
> (Updated Feb. 25, 2016, 10:32 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4779
> https://issues.apache.org/jira/browse/MESOS-4779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore URL query parameters and fragments when determining this
> base name. This enables the fetcher to subsequently examine the
> file extension and extract archives correctly.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/44029/diff/
> 
> 
> Testing
> ---
> 
> ``make check`` on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

[Alexander Rojas]

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




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


There is a second parameter in the `Timer` object which allows you to 
specify a window, once this window is enabled we can get statistics of the run 
beyond how long the last allocation took place (note that allocation varies a 
lot since sometimes it goes through a few elements and other times goes 
throught the whole cluster).



src/master/allocator/mesos/hierarchical.cpp (line 1220)


Add a `CHECK_NOT_NULL`



src/master/allocator/mesos/hierarchical.cpp (line 1236)


While I prefer this kind of constructors. I think I rather go for 
consistency and Mesos code is rather consistent on using parenthesis.

If you look at this file, this will be the only instance of this style (not 
including initializer lists that is).


- Alexander Rojas


On Feb. 26, 2016, 6:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43882/
> ---
> 
> (Updated Feb. 26, 2016, 6:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43882/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43999: Use relative path to create libraries symbolic link.

[Jacob Janco]

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


Ship it!




ran a patched version through our pipeline and our rpm builds look good, thanks!

- Jacob Janco


On Feb. 26, 2016, 8:18 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43999/
> ---
> 
> (Updated Feb. 26, 2016, 8:18 a.m.)
> 
> 
> Review request for mesos, James Peach and Kapil Arya.
> 
> 
> Bugs: MESOS-4774
> https://issues.apache.org/jira/browse/MESOS-4774
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use relative path to create libraries symbolic link.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5 
> 
> Diff: https://reviews.apache.org/r/43999/diff/
> 
> 
> Testing
> ---
> 
> After make install, the library links will be relative path.
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 44108: Added 'Updating the wiki' step to the release guide.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 26, 2016, 11:34 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44108/
> ---
> 
> (Updated Feb. 26, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the recent Mesos Community Sync, it was reminded that we should be keeping 
> the [Mesos Release 
> Planning](https://cwiki.apache.org/confluence/display/MESOS/Mesos+Release+Planning)
>  wiki page up to date.
> This patch adds this step to the release guide so that we don't forget.
> 
> 
> Diffs
> -
> 
>   docs/release-guide.md f0963e750494fcf1f9d80e87bac8dbd6f76adc9f 
> 
> Diff: https://reviews.apache.org/r/44108/diff/
> 
> 
> Testing
> ---
> 
> `rake && rake dev`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 43283: Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1.

[Ian Downes]

> On Feb. 8, 2016, 2:52 p.m., Ian Downes wrote:
> > src/linux/perf.cpp, lines 423-426
> > 
> >
> > Hmmm, I'm not satisfied with this. I thought the new output format was 
> > introduced at a specific kernel version (3.13.0 might not be correct). 
> > Could you please confirm that the 3.10 on CentOS7.1 has the new unit field 
> > but other 3.10 kernels don't.
> 
> haosdent huang wrote:
> The interesting thing here is I saw `unit` appear since 3.14 
> http://lxr.free-electrons.com/source/tools/perf/util/evsel.h?v=3.14#L72 I 
> think I missing something, let me find out more details about this.
> 
> haosdent huang wrote:
> According 
> https://github.com/torvalds/linux/blob/v3.13/tools/perf/builtin-stat.c#L1190-L1206
>  , I think the `perf stat` format for 3.13.0 also is `value,event,cgroup`.
> The `unit` added in this commit which contains since 3.14. 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=410136f5dd96b6013fe6d1011b523b1c247e1ccb
> Let me try create a virtual machine to verify this.
> 
> haosdent huang wrote:
> I try this in 
> 
> ```
> # cat /etc/issue
> Ubuntu 14.04.3 LTS \n \l
> ```
> 
> Kernel is 
> 
> ```
> uname -a
> Linux haosdent 3.13.0-32-generic #57-Ubuntu SMP Tue Jul 15 03:51:08 UTC 
> 2014 x86_64 x86_64 x86_64 GNU/Linux
> 
> 
> The `perf stat` output don't contains unit:
> 
> ```
> perf stat --all-cpus --field-separator , --log-fd 1 --event cycles 
> --cgroup mesos --event task-clock --cgroup mesos -- sleep 0.25
> ,cycles,mesos
> ,task-clock,mesos
> ```

Okay, if I'm understanding you correctly the value,unit,event,cgroup format 
came in 3.14.0, not 3.13.0 as the current code states. Can we not adjust the 
cases to correctly match the precise kernel versions?


- Ian


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


On Feb. 13, 2016, 11:59 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43283/
> ---
> 
> (Updated Feb. 13, 2016, 11:59 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett.
> 
> 
> Bugs: MESOS-4655
> https://issues.apache.org/jira/browse/MESOS-4655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43283/diff/
> 
> 
> Testing
> ---
> 
> This also fix similar error in 
> `CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44110: Updated flag examples to refer to /role instead of stats.json.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On Feb. 27, 2016, 12:55 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44110/
> ---
> 
> (Updated Feb. 27, 2016, 12:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Vinod Kone.
> 
> 
> Bugs: MESOS-4509
> https://issues.apache.org/jira/browse/MESOS-4509
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated flag examples to refer to /role instead of stats.json.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 
>   src/master/flags.cpp 60e085bd5c6689adb625a736edc76e814860ea7d 
>   src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d 
> 
> Diff: https://reviews.apache.org/r/44110/diff/
> 
> 
> Testing
> ---
> 
> make test + manually checked the flags example.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44110: Updated flag examples to refer to /role instead of stats.json.

[Joerg Schad]

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

(Updated Feb. 27, 2016, 12:55 a.m.)


Review request for mesos, Alexander Rojas and Vinod Kone.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Updated flag examples to refer to /role instead of stats.json.


Diffs (updated)
-

  docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 
  src/master/flags.cpp 60e085bd5c6689adb625a736edc76e814860ea7d 
  src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d 

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


Testing
---

make test + manually checked the flags example.


Thanks,

Joerg Schad

Re: Review Request 44110: Updated flag examples to refer to /role instead of stats.json.

[Neil Conway]

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




docs/configuration.md (line 86)


Is this a valid endpoint? "/roles" is a master endpoint but not an agent 
endpoint AFAIK.


- Neil Conway


On Feb. 27, 2016, 12:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44110/
> ---
> 
> (Updated Feb. 27, 2016, 12:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Vinod Kone.
> 
> 
> Bugs: MESOS-4509
> https://issues.apache.org/jira/browse/MESOS-4509
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated flag examples to refer to /role instead of stats.json.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 
>   src/master/flags.cpp 60e085bd5c6689adb625a736edc76e814860ea7d 
>   src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d 
> 
> Diff: https://reviews.apache.org/r/44110/diff/
> 
> 
> Testing
> ---
> 
> make test + manually checked the flags example.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44110: Updated flag examples to refer to /role instead of stats.json.

[Joerg Schad]

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

(Updated Feb. 27, 2016, 12:33 a.m.)


Review request for mesos, Alexander Rojas and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Updated flag examples to refer to /role instead of stats.json.


Diffs (updated)
-

  docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 
  src/master/flags.cpp 60e085bd5c6689adb625a736edc76e814860ea7d 
  src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d 

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


Testing
---

make test + manually checked the flags example.


Thanks,

Joerg Schad

Re: Review Request 44110: Updated flag examples to refer to /role instead of stats.json.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On Feb. 27, 2016, 1:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44110/
> ---
> 
> (Updated Feb. 27, 2016, 1:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Vinod Kone.
> 
> 
> Bugs: MESOS-4509
> https://issues.apache.org/jira/browse/MESOS-4509
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated flag examples to refer to /role instead of stats.json.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 
>   src/master/flags.cpp 60e085bd5c6689adb625a736edc76e814860ea7d 
>   src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d 
> 
> Diff: https://reviews.apache.org/r/44110/diff/
> 
> 
> Testing
> ---
> 
> make test + manually checked the flags example.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 43881: Added allocator metric for the number of allocations to a framework.

[Alexander Rojas]

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



As with the previous one, I don't know whether this metric will help anyone. So 
can you add a use case in the JIRA?


src/tests/hierarchical_allocator_tests.cpp (lines 2432 - 2436)


This looks rather hard to read. I think a better solution would look like 
this:

```c++
std::string metricKey = strings::join("/", 
"allocator/framework_allocations", framework.id());

EXPECT_EQ(1, metrics.values[metricKey]);
```



src/tests/hierarchical_allocator_tests.cpp (lines 2471 - 2480)


Same as above.


- Alexander Rojas


On Feb. 26, 2016, 6:02 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> ---
> 
> (Updated Feb. 26, 2016, 6:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metric for the number of allocations to a framework.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43881/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Alexander Rojas]

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

(Updated Feb. 27, 2016, 1:24 a.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Fixed IO tests.


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


Repository: mesos


Description
---

Under certains circumstances, the future returned by poll is discarded right
after the event is triggered, this causes the event callback to be called
before the discard callback which results in an abort signal being raised
by the libevent library.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_poll.cpp 
461624ca003e97be5ea9cf956d86fc294e6f1bc1 

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


Testing (updated)
---

```bash
# On CentOS 6.7 running in virtualbox
../configure --enable-ssl --enable-libevent
make -j4 check
./3rdpaty/libprocess/libprocess-tests --gtest_repeat=200 
--gtest_break_on_failure
sudo ./bin/mesos-tests.sh 
--gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
--gtest_repeat=1000
```


Thanks,

Alexander Rojas

Re: Review Request 43969: Added test for Appc image fetcher.

[Jie Yu]

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




src/tests/containerizer/provisioner_appc_tests.cpp (line 240)


you can do return JSON::parse(...) here, right?



src/tests/containerizer/provisioner_appc_tests.cpp (lines 241 - 265)


Can you use c++11 string literal here (to avoid backslashes and quotes).

http://en.cppreference.com/w/cpp/language/string_literal

See example here:

https://github.com/apache/mesos/blob/master/src/tests/containerizer/docker_spec_tests.cpp#L104



src/tests/containerizer/provisioner_appc_tests.cpp (line 455)


Can you call it TestAppcImageServer?



src/tests/containerizer/provisioner_appc_tests.cpp (line 479)


Ditto on using c++ string literals. You can also use strings::format to 
avoid inlining varaibles.

```
%s/TestAppcImageServer/image
```



src/tests/containerizer/provisioner_appc_tests.cpp (line 510)


We might want to serve multiple images. Can you make it more general so 
that we can serve any files under 'server' directory?



src/tests/containerizer/provisioner_appc_tests.cpp (line 528)


We avoid static non-pod. Can you just use:
```
const string serverDir = path::join(os::getcwd(), "server");
```



src/tests/containerizer/provisioner_appc_tests.cpp (line 550)


We don't use 'override' in our code base. Can you remove them for now for 
consistency?



src/tests/containerizer/provisioner_appc_tests.cpp (lines 555 - 558)


I'd like this to be called by the test. We might want to prepare multiple 
images to test the code path of fetching dependencies.


- Jie Yu


On Feb. 24, 2016, 11:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43969/
> ---
> 
> (Updated Feb. 24, 2016, 11:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4748
> https://issues.apache.org/jira/browse/MESOS-4748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added simple appc Fetcher test with mock HTTP image server.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 
> 
> Diff: https://reviews.apache.org/r/43969/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Neil Conway]

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

(Updated Feb. 27, 2016, 12:10 a.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Add comments.


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


Repository: mesos


Description
---

Scalar resource values are represented using floating point. As a result, users
could see unexpected results when accepting offers and making reservations for
fractional resources: values like "0.1" cannot be precisely represented using
standard floating point, and the resource values returned to frameworks might
contain an unpredictable amount of roundoff error.

This commit adjusts the master to use fixed-point when doing internal
computations on scalar resource values. The fixed-point format only supports
three decimal digits of precision: that is, fractional resource values like
"0.001" will be supported, but "0.0001" will not be.


Diffs (updated)
-

  docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
  docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 
  include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 
  include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 
  src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 
  src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
  src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 
  src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 

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


Testing
---

make check

Manually verified that some of the floating point oddities in 
https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
is applied, although I wasn't able to reproduce the crash described in that 
ticket.

REVIEW NOTES:

* We don't currently emit a warning when discarding additional digits of 
precision from input scalar resource values. Should we? That would require 
identifying all the points where a resource value is seemed to be 
"user-provided", and also runs the risk of generating a ton of log messages 
when an old framework is used.
* Similarly, if the user gives us a resource value and we don't do anything to 
it, we won't discard any additional precision that appears in the value -- the 
precision only gets discarded when we apply an operator like `+` or `-`. 
Unclear if we should trim additional precision from all scalar resource values 
more aggressively.

PERFORMANCE:

This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
applied (optimized build on my laptop):

```
[ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
Using 2000 slaves and 200 frameworks
round 0 allocate took 2.192425secs to make 200 offers
round 1 allocate took 2.221243secs to make 200 offers
round 2 allocate took 2.236314secs to make 200 offers
round 3 allocate took 2.224045secs to make 200 offers
round 4 allocate took 2.232822secs to make 200 offers
round 5 allocate took 2.264807secs to make 200 offers
round 6 allocate took 2.224853secs to make 200 offers
round 7 allocate took 2.224829secs to make 200 offers
round 8 allocate took 2.24862secs to make 200 offers
round 9 allocate took 2.2556secs to make 200 offers
round 10 allocate took 2.256616secs to make 200 offers
```

And after applying this RR:

```
[ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
Using 2000 slaves and 200 frameworks
round 0 allocate took 2.267919secs to make 200 offers
round 1 allocate took 2.202843secs to make 200 offers
round 2 allocate took 2.20426secs to make 200 offers
round 3 allocate took 2.263887secs to make 200 offers
round 4 allocate took 2.266237secs to make 200 offers
round 5 allocate took 2.276957secs to make 200 offers
round 6 allocate took 2.291821secs to make 200 offers
round 7 allocate took 2.261839secs to make 200 offers
round 8 allocate took 2.325696secs to make 200 offers
round 9 allocate took 2.310469secs to make 200 offers
round 10 allocate took 2.21802secs to make 200 offers
```

Which suggests to me that any performance hit is pretty minimal.


Thanks,

Neil Conway

Re: Review Request 44001: CMake: Add MasterConfigure for master executable build.

[Alex Clemmer]

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




src/master/cmake/MasterConfigure.cmake (line 49)


Which of these do we actually need for the master? It looks like we're 
copying this directly from the stuff in `SlaveConfigure.cmake`, is that right?

If so, two notes:

(1) The `AGENT_INCLUDE_DIRS`, `AGENT_DEPENDENCIES`, and so on, are actually 
misnomers -- it should really be called `MESOS_INCLUDE_DIRS`, _etc_., because 
it is building libmesos, not the slave.
(2) Therefore, I recommend renaming those variables to be `MESOS_*` instead 
of `AGENT_*`, and then just passing the `MESOS_INCLUDE_DIRS` into the 
`MASTER_INCLUDE_DIRS` instead of copying them everywhere. Possibly this 
involves moving those definitions to `MesosConfigure.cmake` instead, as the 
libmesos configuration really doesn't belong in `SlaveConfigure.cmake`.

My rationale for point 2 here, is that we are just copying around the smae 
include paths in a bunch of places now, and this is bad. Every time we add a 
directory, we'll need to add it to two places. Why not just add it to the 
libmesos configuration once, and have both other things consume that 
configuration instead?

Thoughts?


- Alex Clemmer


On Feb. 25, 2016, 8:53 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44001/
> ---
> 
> (Updated Feb. 25, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-4773
> https://issues.apache.org/jira/browse/MESOS-4773
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Add MasterConfigure for master executable build.
> 
> 
> Diffs
> -
> 
>   src/master/cmake/MasterConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44001/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/common/values.cpp (line 62)


Can we add a comment explaining why we use this quotient, remainder 
approach?


- Joris Van Remoortere


On Feb. 26, 2016, 11:52 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43635/
> ---
> 
> (Updated Feb. 26, 2016, 11:52 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-4687
> https://issues.apache.org/jira/browse/MESOS-4687
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Scalar resource values are represented using floating point. As a result, 
> users
> could see unexpected results when accepting offers and making reservations for
> fractional resources: values like "0.1" cannot be precisely represented using
> standard floating point, and the resource values returned to frameworks might
> contain an unpredictable amount of roundoff error.
> 
> This commit adjusts the master to use fixed-point when doing internal
> computations on scalar resource values. The fixed-point format only supports
> three decimal digits of precision: that is, fractional resource values like
> "0.001" will be supported, but "0.0001" will not be.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
>   docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 
>   include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 
>   include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 
>   src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 
>   src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 
> 
> Diff: https://reviews.apache.org/r/43635/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manually verified that some of the floating point oddities in 
> https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
> is applied, although I wasn't able to reproduce the crash described in that 
> ticket.
> 
> REVIEW NOTES:
> 
> * We don't currently emit a warning when discarding additional digits of 
> precision from input scalar resource values. Should we? That would require 
> identifying all the points where a resource value is seemed to be 
> "user-provided", and also runs the risk of generating a ton of log messages 
> when an old framework is used.
> * Similarly, if the user gives us a resource value and we don't do anything 
> to it, we won't discard any additional precision that appears in the value -- 
> the precision only gets discarded when we apply an operator like `+` or `-`. 
> Unclear if we should trim additional precision from all scalar resource 
> values more aggressively.
> 
> PERFORMANCE:
> 
> This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
> applied (optimized build on my laptop):
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.192425secs to make 200 offers
> round 1 allocate took 2.221243secs to make 200 offers
> round 2 allocate took 2.236314secs to make 200 offers
> round 3 allocate took 2.224045secs to make 200 offers
> round 4 allocate took 2.232822secs to make 200 offers
> round 5 allocate took 2.264807secs to make 200 offers
> round 6 allocate took 2.224853secs to make 200 offers
> round 7 allocate took 2.224829secs to make 200 offers
> round 8 allocate took 2.24862secs to make 200 offers
> round 9 allocate took 2.2556secs to make 200 offers
> round 10 allocate took 2.256616secs to make 200 offers
> ```
> 
> And after applying this RR:
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.267919secs to make 200 offers
> round 1 allocate took 2.202843secs to make 200 offers
> round 2 allocate took 2.20426secs to make 200 offers
> round 3 allocate took 2.263887secs to make 200 offers
> round 4 allocate took 2.266237secs to make 200 offers
> round 5 allocate took 2.276957secs to make 200 offers
> round 6 allocate took 2.291821secs to make 200 offers
> round 7 allocate took 2.261839secs to make 200 offers
> round 8 allocate took 2.325696secs to make 200 offers
> round 9 allocate took 2.310469secs to make 

Re: Review Request 41092: CMake: Added CMake file for agent executable build.

[Alex Clemmer]

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




src/slave/CMakeLists.txt (line 27)


I liked what you did with the master -- there should be only a `main.cpp` 
file here, so what do you think of just making this 
`add_executable(${AGENT_TARGET} main.cpp)`?



src/slave/CMakeLists.txt (line 33)


Same comment as the master review -- I recommend putting the `*_TARGET`s 
inside a single variable inside `SlaveConfigure.cmake`, and simply calling 
`add_dependencies(${AGENT_TARGET} ${AGENT_DEPENDENCIES})` or whatever. This 
should be very similar to the slave.



src/slave/CMakeLists.txt (line 37)


See comment above and comment in the master review for my recommendations 
here -- thinking it woudl be better to just put this into a single variable in 
`SlaveConfigure.cmake`.


- Alex Clemmer


On Feb. 13, 2016, 4:23 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41092/
> ---
> 
> (Updated Feb. 13, 2016, 4:23 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CMake file for agent executable build.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt PRE-CREATION 
>   src/slave/cmake/SlaveConfigure.cmake 
> fbdfdaa27fbd8c7429861eea5baf401a221f748b 
> 
> Diff: https://reviews.apache.org/r/41092/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Neil Conway]

> On Feb. 26, 2016, 9:40 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 61
> > 
> >
> > let's pull out into a constant as discussed offline.

Per discussion, we aren't going to do this for now (stupid `constexpr`).


- Neil


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


On Feb. 26, 2016, 11:52 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43635/
> ---
> 
> (Updated Feb. 26, 2016, 11:52 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-4687
> https://issues.apache.org/jira/browse/MESOS-4687
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Scalar resource values are represented using floating point. As a result, 
> users
> could see unexpected results when accepting offers and making reservations for
> fractional resources: values like "0.1" cannot be precisely represented using
> standard floating point, and the resource values returned to frameworks might
> contain an unpredictable amount of roundoff error.
> 
> This commit adjusts the master to use fixed-point when doing internal
> computations on scalar resource values. The fixed-point format only supports
> three decimal digits of precision: that is, fractional resource values like
> "0.001" will be supported, but "0.0001" will not be.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
>   docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 
>   include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 
>   include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 
>   src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 
>   src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 
> 
> Diff: https://reviews.apache.org/r/43635/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manually verified that some of the floating point oddities in 
> https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
> is applied, although I wasn't able to reproduce the crash described in that 
> ticket.
> 
> REVIEW NOTES:
> 
> * We don't currently emit a warning when discarding additional digits of 
> precision from input scalar resource values. Should we? That would require 
> identifying all the points where a resource value is seemed to be 
> "user-provided", and also runs the risk of generating a ton of log messages 
> when an old framework is used.
> * Similarly, if the user gives us a resource value and we don't do anything 
> to it, we won't discard any additional precision that appears in the value -- 
> the precision only gets discarded when we apply an operator like `+` or `-`. 
> Unclear if we should trim additional precision from all scalar resource 
> values more aggressively.
> 
> PERFORMANCE:
> 
> This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
> applied (optimized build on my laptop):
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.192425secs to make 200 offers
> round 1 allocate took 2.221243secs to make 200 offers
> round 2 allocate took 2.236314secs to make 200 offers
> round 3 allocate took 2.224045secs to make 200 offers
> round 4 allocate took 2.232822secs to make 200 offers
> round 5 allocate took 2.264807secs to make 200 offers
> round 6 allocate took 2.224853secs to make 200 offers
> round 7 allocate took 2.224829secs to make 200 offers
> round 8 allocate took 2.24862secs to make 200 offers
> round 9 allocate took 2.2556secs to make 200 offers
> round 10 allocate took 2.256616secs to make 200 offers
> ```
> 
> And after applying this RR:
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.267919secs to make 200 offers
> round 1 allocate took 2.202843secs to make 200 offers
> round 2 allocate took 2.20426secs to make 200 offers
> round 3 allocate took 2.263887secs to make 200 offers
> round 4 allocate took 2.266237secs to make 200 offers
> round 5 allocate took 2.276957secs to make 200 offers
> round 6 allocate took 2.291821secs to make 200 offers
> round 7 allocate took 2.261839secs to make 200 

Re: Review Request 44003: CMake: Add CMakeLists for master executable build.

[Alex Clemmer]

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




src/master/CMakeLists.txt (line 17)


It looks like there aren't actually any source files to add, so perhaps we 
should do away with this comment?

I recommend moving the comment `THE PROCESS SLAVE...` from a couple lines 
below, to this spot. That way you get rid of the awkward indentation below.



src/master/CMakeLists.txt (line 20)


When we move the comment below, could we also get rid of this space?



src/master/CMakeLists.txt (line 29)


A list of targets for dependencies should probably be maintained in the 
`*Configure.cmake` scripts, _i.e._, we should be writing something like:

```
add_dependencies(${MASTER_TARGET} ${MASTER_DEPENDENCIES})
```

If I did something different in another place, I was being very sloppy 
indeed!



src/master/CMakeLists.txt (line 33)


Same comment as the dependencies -- we should make this something like the 
following, much like we did with the agent.

```
target_link_libraries(${MASTER_TARGET} ${MASTER_LIBS})
```


- Alex Clemmer


On Feb. 25, 2016, 2:10 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44003/
> ---
> 
> (Updated Feb. 25, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-4773
> https://issues.apache.org/jira/browse/MESOS-4773
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Add CMakeLists for master executable build.
> 
> 
> Diffs
> -
> 
>   src/master/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44003/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Neil Conway]

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

(Updated Feb. 26, 2016, 11:52 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Address code review comments.


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


Repository: mesos


Description
---

Scalar resource values are represented using floating point. As a result, users
could see unexpected results when accepting offers and making reservations for
fractional resources: values like "0.1" cannot be precisely represented using
standard floating point, and the resource values returned to frameworks might
contain an unpredictable amount of roundoff error.

This commit adjusts the master to use fixed-point when doing internal
computations on scalar resource values. The fixed-point format only supports
three decimal digits of precision: that is, fractional resource values like
"0.001" will be supported, but "0.0001" will not be.


Diffs (updated)
-

  docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
  docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 
  include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 
  include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 
  src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 
  src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
  src/master/allocator/mesos/hierarchical.cpp 
5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
  src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
  src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 
  src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 

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


Testing
---

make check

Manually verified that some of the floating point oddities in 
https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
is applied, although I wasn't able to reproduce the crash described in that 
ticket.

REVIEW NOTES:

* We don't currently emit a warning when discarding additional digits of 
precision from input scalar resource values. Should we? That would require 
identifying all the points where a resource value is seemed to be 
"user-provided", and also runs the risk of generating a ton of log messages 
when an old framework is used.
* Similarly, if the user gives us a resource value and we don't do anything to 
it, we won't discard any additional precision that appears in the value -- the 
precision only gets discarded when we apply an operator like `+` or `-`. 
Unclear if we should trim additional precision from all scalar resource values 
more aggressively.

PERFORMANCE:

This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
applied (optimized build on my laptop):

```
[ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
Using 2000 slaves and 200 frameworks
round 0 allocate took 2.192425secs to make 200 offers
round 1 allocate took 2.221243secs to make 200 offers
round 2 allocate took 2.236314secs to make 200 offers
round 3 allocate took 2.224045secs to make 200 offers
round 4 allocate took 2.232822secs to make 200 offers
round 5 allocate took 2.264807secs to make 200 offers
round 6 allocate took 2.224853secs to make 200 offers
round 7 allocate took 2.224829secs to make 200 offers
round 8 allocate took 2.24862secs to make 200 offers
round 9 allocate took 2.2556secs to make 200 offers
round 10 allocate took 2.256616secs to make 200 offers
```

And after applying this RR:

```
[ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
Using 2000 slaves and 200 frameworks
round 0 allocate took 2.267919secs to make 200 offers
round 1 allocate took 2.202843secs to make 200 offers
round 2 allocate took 2.20426secs to make 200 offers
round 3 allocate took 2.263887secs to make 200 offers
round 4 allocate took 2.266237secs to make 200 offers
round 5 allocate took 2.276957secs to make 200 offers
round 6 allocate took 2.291821secs to make 200 offers
round 7 allocate took 2.261839secs to make 200 offers
round 8 allocate took 2.325696secs to make 200 offers
round 9 allocate took 2.310469secs to make 200 offers
round 10 allocate took 2.21802secs to make 200 offers
```

Which suggests to me that any performance hit is pretty minimal.


Thanks,

Neil Conway

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Joris Van Remoortere]

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



This breaks the IO based tests in Libprocess.

- Joris Van Remoortere


On Feb. 26, 2016, 9:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 26, 2016, 9:38 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 44113: Cleaned up assertions in test cases.

[Neil Conway]

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

Review request for mesos, Joris Van Remoortere and Michael Park.


Repository: mesos


Description
---

Ensure that `EXPECT_EQ` and friends use "expected, actual" as the
order of arguments.


Diffs
-

  src/tests/containerizer/docker_spec_tests.cpp 
5799dc9c871c6ccaddb209ab7ec1112b192f3d41 
  src/tests/containerizer/docker_tests.cpp 
620819330847a10d9dcd817968df9d2b180a9a29 
  src/tests/containerizer/fs_tests.cpp 29e43877612fa151e6f6d79268a7411272a7bfeb 
  src/tests/containerizer/provisioner_backend_tests.cpp 
25b28ef8fa5aae81e8dd0c9e33df4160dd912ce8 
  src/tests/module_tests.cpp 121d65337bdf29f6049ac44bfd903a1f5ea1a09d 
  src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
  src/tests/sorter_tests.cpp 22bc618f9b4958fbd8e6c5dfee94b26fe13ec47a 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43416: Windows: Removed ambiguous call to `::write`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 26, 2016, 9:23 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43416/
> ---
> 
> (Updated Feb. 26, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Removed ambiguous call to `::write`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
> 88b355e09f76f0412c74ad69556572f0079deb8f 
> 
> Diff: https://reviews.apache.org/r/43416/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On Feb. 26, 2016, 9:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 26, 2016, 9:38 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 26, 2016, 10:21 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42733/
> ---
> 
> (Updated Feb. 26, 2016, 10:21 p.m.)
> 
> 
> Review request for mesos, Michael Park, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-3940
> https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checks for presence of `ReservationInfo.principal`.
> 
> The `ReservationInfo.principal` field was recently made `optional`. There 
> remain a couple places in the code that assume this field is always set. This 
> patch uses `has_principal()` before this field is accessed to ensure safety.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 
> 
> Diff: https://reviews.apache.org/r/42733/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44026: Moved future tests into future_tests.cpp.

[Cong Wang]

> On Feb. 26, 2016, 1:45 a.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/tests/process_tests.cpp, lines 1764-1776
> > 
> >
> > Should this be moved to `future_tests.cpp` too?

Yes, it should, because this is a pure Future test, not related to process.


- Cong


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


On Feb. 25, 2016, 5:59 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44026/
> ---
> 
> (Updated Feb. 25, 2016, 5:59 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved future tests into future_tests.cpp.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> ded1aaed25876d45e33b4a27fffc6a5c46ca92f5 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> e9bf80ee69f4add299cb828ed3245ac07398943c 
> 
> Diff: https://reviews.apache.org/r/44026/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Neil Conway]

> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote:
> > src/common/values.cpp, line 67
> > 
> >
> > Let's add check on overflow; it will be helpful if scalar value was 
> > big. Scalar is a general type, there maybe used with a big value, e.g. 
> > total size of distributed filesystem.
> 
> Neil Conway wrote:
> What should we do in case of overflow?
> 
> Note that we don't check for overflow in operator+ (or for negative 
> result values in operator-)...
> 
> Klaus Ma wrote:
> `CHECK` or warning log should be fine, it only improves the debugability; 
> it different with `operator+` because `max_double/2` should be big enough for 
> us; but for `double * 1000 -> long`, I'm not sure of that.
> 
> And I think we need to use `long long` or `int64` instead of `long`; 
> `long` is not garantee to be 64bit in c++.
> 
> Neil Conway wrote:
> re: `long`, good point -- we only officially support 64-bit OSX, Linux, 
> and Windows, but `long` is 32-bit on Windows.

After discussing this with a few folks offline, our feeling was that the range 
of resource values we can represent without overflow should be sufficient for 
any reasonable purpose for a considerable length of time. Hopefully we'll have 
switched to a fixed point format before people use Mesos to manage > 10s of 
exabytes as a single resource :)


- Neil


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


On Feb. 19, 2016, 10:27 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43635/
> ---
> 
> (Updated Feb. 19, 2016, 10:27 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-4687
> https://issues.apache.org/jira/browse/MESOS-4687
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Scalar resource values are represented using floating point. As a result, 
> users
> could see unexpected results when accepting offers and making reservations for
> fractional resources: values like "0.1" cannot be precisely represented using
> standard floating point, and the resource values returned to frameworks might
> contain an unpredictable amount of roundoff error.
> 
> This commit adjusts the master to use fixed-point when doing internal
> computations on scalar resource values. The fixed-point format only supports
> three decimal digits of precision: that is, fractional resource values like
> "0.001" will be supported, but "0.0001" will not be.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
>   docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 
>   include/mesos/mesos.proto 636550f255a122d7f714dbd58f587bea8221b461 
>   include/mesos/v1/mesos.proto 1d5af88a343fe9d81688bb3e83aa997ccba7d030 
>   src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 
>   src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 
> 
> Diff: https://reviews.apache.org/r/43635/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manually verified that some of the floating point oddities in 
> https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
> is applied, although I wasn't able to reproduce the crash described in that 
> ticket.
> 
> REVIEW NOTES:
> 
> * We don't currently emit a warning when discarding additional digits of 
> precision from input scalar resource values. Should we? That would require 
> identifying all the points where a resource value is seemed to be 
> "user-provided", and also runs the risk of generating a ton of log messages 
> when an old framework is used.
> * Similarly, if the user gives us a resource value and we don't do anything 
> to it, we won't discard any additional precision that appears in the value -- 
> the precision only gets discarded when we apply an operator like `+` or `-`. 
> Unclear if we should trim additional precision from all scalar resource 
> values more aggressively.
> 
> PERFORMANCE:
> 
> This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
> applied (optimized build on my laptop):
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.192425secs to make 200 offers
> round 1 allocate took 2.221243secs to make 200 

Re: Review Request 43855: Added Appc fetcher support to store.

[Jojy Varghese]

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

(Updated Feb. 26, 2016, 10:42 p.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

This change allows store to fetch an image using Appc image fetcher when an
image is not found in the cache. It also recursively fetches the dependencies
for the image.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
4b3829175f57fb9aea2478040d96f2f127cbc551 
  src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e 

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


Testing
---

make check; local image server.


Thanks,

Jojy Varghese

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/common/values.cpp (lines 60 - 62)


How about if we re-construct the double from its whole and decimal parts 
using `/1000` and `%1000`?


- Joris Van Remoortere


On Feb. 19, 2016, 10:27 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43635/
> ---
> 
> (Updated Feb. 19, 2016, 10:27 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-4687
> https://issues.apache.org/jira/browse/MESOS-4687
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Scalar resource values are represented using floating point. As a result, 
> users
> could see unexpected results when accepting offers and making reservations for
> fractional resources: values like "0.1" cannot be precisely represented using
> standard floating point, and the resource values returned to frameworks might
> contain an unpredictable amount of roundoff error.
> 
> This commit adjusts the master to use fixed-point when doing internal
> computations on scalar resource values. The fixed-point format only supports
> three decimal digits of precision: that is, fractional resource values like
> "0.001" will be supported, but "0.0001" will not be.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
>   docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 
>   include/mesos/mesos.proto 636550f255a122d7f714dbd58f587bea8221b461 
>   include/mesos/v1/mesos.proto 1d5af88a343fe9d81688bb3e83aa997ccba7d030 
>   src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 
>   src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 
> 
> Diff: https://reviews.apache.org/r/43635/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manually verified that some of the floating point oddities in 
> https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
> is applied, although I wasn't able to reproduce the crash described in that 
> ticket.
> 
> REVIEW NOTES:
> 
> * We don't currently emit a warning when discarding additional digits of 
> precision from input scalar resource values. Should we? That would require 
> identifying all the points where a resource value is seemed to be 
> "user-provided", and also runs the risk of generating a ton of log messages 
> when an old framework is used.
> * Similarly, if the user gives us a resource value and we don't do anything 
> to it, we won't discard any additional precision that appears in the value -- 
> the precision only gets discarded when we apply an operator like `+` or `-`. 
> Unclear if we should trim additional precision from all scalar resource 
> values more aggressively.
> 
> PERFORMANCE:
> 
> This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
> applied (optimized build on my laptop):
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.192425secs to make 200 offers
> round 1 allocate took 2.221243secs to make 200 offers
> round 2 allocate took 2.236314secs to make 200 offers
> round 3 allocate took 2.224045secs to make 200 offers
> round 4 allocate took 2.232822secs to make 200 offers
> round 5 allocate took 2.264807secs to make 200 offers
> round 6 allocate took 2.224853secs to make 200 offers
> round 7 allocate took 2.224829secs to make 200 offers
> round 8 allocate took 2.24862secs to make 200 offers
> round 9 allocate took 2.2556secs to make 200 offers
> round 10 allocate took 2.256616secs to make 200 offers
> ```
> 
> And after applying this RR:
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.267919secs to make 200 offers
> round 1 allocate took 2.202843secs to make 200 offers
> round 2 allocate took 2.20426secs to make 200 offers
> round 3 allocate took 2.263887secs to make 200 offers
> round 4 allocate took 2.266237secs to make 200 offers
> round 5 allocate took 2.276957secs to make 200 offers
> round 6 allocate took 2.291821secs to make 200 offers
> round 7 allocate took 2.261839secs to make 200 offers
> round 8 allocate took 2.325696secs to make 200 offers
> round 9 

Re: Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Greg Mann]

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

(Updated Feb. 26, 2016, 10:21 p.m.)


Review request for mesos, Michael Park, Neil Conway, and Vinod Kone.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added checks for presence of `ReservationInfo.principal`.

The `ReservationInfo.principal` field was recently made `optional`. There 
remain a couple places in the code that assume this field is always set. This 
patch uses `has_principal()` before this field is accessed to ensure safety.


Diffs (updated)
-

  src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
  src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
  src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 43552: Added a support/push-reviews.py script to push reviews upstream.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Feb. 24, 2016, 7:23 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43552/
> ---
> 
> (Updated Feb. 24, 2016, 7:23 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Kevin Klues, and Michael Park.
> 
> 
> Bugs: MESOS-3929
> https://issues.apache.org/jira/browse/MESOS-3929
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This script allows committers to push locally applied review chain to the ASF
> git repo and mark the reviews as submitted.
> 
> 
> Diffs
> -
> 
>   support/push-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43552/diff/
> 
> 
> Testing
> ---
> 
> Tested locally.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 43796: Added documentation for `cgroups/net_cls` isolator.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 22, 2016, 6:26 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43796/
> ---
> 
> (Updated Feb. 22, 2016, 6:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Bugs: MESOS-4660
> https://issues.apache.org/jira/browse/MESOS-4660
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for `cgroups/net_cls` isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 87f145cd957dcb8fd3188c866212b417f0ab6296 
> 
> Diff: https://reviews.apache.org/r/43796/diff/
> 
> 
> Testing
> ---
> 
> Built the web-site using docker, and proof read the website and links on 
> localhost.
> 
> Verified all the links embedded in markdown work.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Vinod Kone]

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


Fix it, then Ship it!





src/common/resources.cpp (line 68)


no need for else if, just do an if.



src/v1/resources.cpp (line 70)


ditto.


- Vinod Kone


On Feb. 26, 2016, 6:31 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42733/
> ---
> 
> (Updated Feb. 26, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos, Michael Park, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-3940
> https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checks for presence of `ReservationInfo.principal`.
> 
> The `ReservationInfo.principal` field was recently made `optional`. There 
> remain a couple places in the code that assume this field is always set. This 
> patch uses `has_principal()` before this field is accessed to ensure safety.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 
> 
> Diff: https://reviews.apache.org/r/42733/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43906: CMake: Added files to be built as part of libmesos.

[Alex Naparu]

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


Ship it!




Ship It!

- Alex Naparu


On Feb. 25, 2016, 7:17 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43906/
> ---
> 
> (Updated Feb. 25, 2016, 7:17 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Added files to be built as part of libmesos.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 5cf0ec8c475839ad8717192a37f01546cbcccd7a 
> 
> Diff: https://reviews.apache.org/r/43906/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43908: Stout:[2/2] Added significant test coverage of `os::rmdir`.

[Alex Naparu]

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


Ship it!




Ship It!

- Alex Naparu


On Feb. 25, 2016, 7:40 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43908/
> ---
> 
> (Updated Feb. 25, 2016, 7:40 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[2/2] Added significant test coverage of `os::rmdir`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 756aa29ed134bf10a645af7f6562f86dc8e488f5 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 3c65d0422dc6e198180d53d1c9e6cb2839137434 
>   3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
> d0592ef8a774d380e9df66b7e623eb72b29a28b3 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422 
> 
> Diff: https://reviews.apache.org/r/43908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43635: Changed scalar resources to use fixed-point internally.

[Joris Van Remoortere]

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




src/common/values.cpp (line 55)


`std::llround`



src/common/values.cpp (line 61)


let's pull out into a constant as discussed offline.



src/tests/resources_tests.cpp (line 859)


expected, actual ?



src/tests/resources_tests.cpp (lines 1592 - 1605)


Can you add another test like this that adds 100K times, and then subtracts 
100K times? and checks between the top and bottom?


- Joris Van Remoortere


On Feb. 19, 2016, 10:27 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43635/
> ---
> 
> (Updated Feb. 19, 2016, 10:27 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-4687
> https://issues.apache.org/jira/browse/MESOS-4687
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Scalar resource values are represented using floating point. As a result, 
> users
> could see unexpected results when accepting offers and making reservations for
> fractional resources: values like "0.1" cannot be precisely represented using
> standard floating point, and the resource values returned to frameworks might
> contain an unpredictable amount of roundoff error.
> 
> This commit adjusts the master to use fixed-point when doing internal
> computations on scalar resource values. The fixed-point format only supports
> three decimal digits of precision: that is, fractional resource values like
> "0.001" will be supported, but "0.0001" will not be.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
>   docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 
>   include/mesos/mesos.proto 636550f255a122d7f714dbd58f587bea8221b461 
>   include/mesos/v1/mesos.proto 1d5af88a343fe9d81688bb3e83aa997ccba7d030 
>   src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 
>   src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 
> 
> Diff: https://reviews.apache.org/r/43635/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manually verified that some of the floating point oddities in 
> https://issues.apache.org/jira/browse/MESOS-4071 do not occur when this patch 
> is applied, although I wasn't able to reproduce the crash described in that 
> ticket.
> 
> REVIEW NOTES:
> 
> * We don't currently emit a warning when discarding additional digits of 
> precision from input scalar resource values. Should we? That would require 
> identifying all the points where a resource value is seemed to be 
> "user-provided", and also runs the risk of generating a ton of log messages 
> when an old framework is used.
> * Similarly, if the user gives us a resource value and we don't do anything 
> to it, we won't discard any additional precision that appears in the value -- 
> the precision only gets discarded when we apply an operator like `+` or `-`. 
> Unclear if we should trim additional precision from all scalar resource 
> values more aggressively.
> 
> PERFORMANCE:
> 
> This is the performance of the `DeclineOffers` benchmark WITHOUT this RR 
> applied (optimized build on my laptop):
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.192425secs to make 200 offers
> round 1 allocate took 2.221243secs to make 200 offers
> round 2 allocate took 2.236314secs to make 200 offers
> round 3 allocate took 2.224045secs to make 200 offers
> round 4 allocate took 2.232822secs to make 200 offers
> round 5 allocate took 2.264807secs to make 200 offers
> round 6 allocate took 2.224853secs to make 200 offers
> round 7 allocate took 2.224829secs to make 200 offers
> round 8 allocate took 2.24862secs to make 200 offers
> round 9 allocate took 2.2556secs to make 200 offers
> round 10 allocate took 2.256616secs to make 200 offers
> ```
> 
> And after applying this RR:
> 
> ```
> [ RUN  ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers
> Using 2000 slaves and 200 frameworks
> round 0 allocate took 2.267919secs to make 200 offers
> round 1 allocate took 2.202843secs to make 200 offers
> round 2 allocate 

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Alexander Rojas]

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

(Updated Feb. 26, 2016, 10:38 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Fixed comment formatting.


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


Repository: mesos


Description
---

Under certains circumstances, the future returned by poll is discarded right
after the event is triggered, this causes the event callback to be called
before the discard callback which results in an abort signal being raised
by the libevent library.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_poll.cpp 
461624ca003e97be5ea9cf956d86fc294e6f1bc1 

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


Testing
---

```bash
# On CentOS 6.7 running in virtualbox
../configure --enable-ssl --enable-libevent
make -j4 check
sudo ./bin/mesos-tests.sh 
--gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
--gtest_repeat=1000
```


Thanks,

Alexander Rojas

Re: Review Request 43907: Stout:[1/2] Fix error reporting bug in `os::rmdir`.

[Alex Naparu]

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


Ship it!




Ship It!

- Alex Naparu


On Feb. 25, 2016, 7:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43907/
> ---
> 
> (Updated Feb. 25, 2016, 7:41 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Fix error reporting bug in `os::rmdir`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
> 
> Diff: https://reviews.apache.org/r/43907/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43938: Required jsonifying of generic protobuf to be explicit opt-in [stout].

[Neil Conway]

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


Fix it, then Ship it!





3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 624)


This needs a comment, I think.



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 625)


Is this the best name for this type? Not sure there's a better name, but 
`JSON::Protobuf` doesn't necessarily imply to me that the type names a certain 
representation of `google::protobuf::Message`. What about `JSON::RawProtobuf`?


- Neil Conway


On Feb. 26, 2016, 8:39 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43938/
> ---
> 
> (Updated Feb. 26, 2016, 8:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-4754
> https://issues.apache.org/jira/browse/MESOS-4754
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See JIRA ticket.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> eb5502c4987da5593169a86b21f60c01aa5b5170 
>   3rdparty/libprocess/3rdparty/stout/include/stout/representation.hpp 
> 22f70f7536c6f5d24ff59228d8ba7bf41319fd4a 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> 8dd9cfd3e7d1e3ab4ace87066a43a3094b776d82 
> 
> Diff: https://reviews.apache.org/r/43938/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 44101: Added links to the operator endpoint doc pages.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added links to the operator endpoint doc pages.


Diffs
-

  docs/logging.md 50a95ba5fedcb5b803e183bea8243e2e7750aa3e 
  docs/mesos-containerizer.md 87f145cd957dcb8fd3188c866212b417f0ab6296 
  docs/network-monitoring.md 57b859e9292dc22613e235710312d1a33f58e0c0 
  docs/networking-for-mesos-managed-containers.md 
f2fbea54cc3a7f1727c53629c86feb25d7f5fd68 
  docs/persistent-volume.md f772405ca9a27eb47b1f0aad9f54df5b47e88cbc 
  docs/quota.md 48dcd2a79202a6d5fbee86c6ae458cbdd71a22ad 
  docs/reservation.md cbf0a085dfe4e514aede11322f6025c7ff376207 
  docs/roles.md 84e5b7eaa4cf5cd3e7ebd6151928210be93d1d56 
  docs/sandbox.md 276e1126f495e7af15afd4f4ad8c63beb40db739 

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


Testing
---

Previewed links in site-docker and verified that they work.


Thanks,

Neil Conway

Re: Review Request 38074: Calculate schedule latency with trace events

[Cong Wang]

> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.hpp, line 97
> > 
> >
> > linux/cgroups has an internal {{Result cgroup(pid_t pid, const 
> > string& subsystem)}} that is wrapped for cpu, cpuacct, etc. Can you add an 
> > wrapper for the perf_event cgroup and use that? Yes, it may be less 
> > efficient than looking at the hashmap, but it'll reuse the code and keep 
> > consistency.

We need to cache that value before sampling the events, we can't call cgroup() 
each time when we lookup the pid, because cgroup() always reads from the cgroup 
tasks file.


> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, lines 472-482
> > 
> >
> > remove this, see earlier comment.

Removed after switching to pid -> cgroup mapping.


> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 486
> > 
> >
> > Suggest that you use a hashmap, i.e., just directly 
> > store the cgroup for each pid. Slight tradeoff in size but then you can 
> > avoid findCgroupByPid() and simplify the code.

Done. But note that with pid namespace enabled we could have duplicated pid's 
from different containers. I just add a TODO there.


> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 502
> > 
> >
> > You don't actually use this as a map? It would be much clearer to use a 
> > vector and explicitly sort with a comparision function on the timestamp. 
> > Suggest just calling it events.
> > 
> > ```
> >   events.push_back(event);
> > }
> > 
> > std::sort(begin(events), end(events), [](...){});
> > ```

I don't see any advantage of sorting it to just using std::map, therefore I 
keep as it is.


> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 547
> > 
> >
> > Are states enumerated somewhere?

Ideally kernel should export these values, but it doesn't. We could enumerate 
by ourselves, but here we only care about RUNNING which is 0, so...


> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 560
> > 
> >
> > sort the latencies in schedLatency here?

I don't see any advantage to sort them here than later.


> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, lines 625-627
> > 
> >
> > How expensive is this...?

Depends on the workload, we could have tens of thousands events on a busy 
system.


> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, lines 629-632
> > 
> >
> > Users have different kernels, code should determine the version at run 
> > time and act accordingly.

That means we would need two separated code to handle two kernel versions.


- Cong


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


On Sept. 30, 2015, 12:15 a.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38074/
> ---
> 
> (Updated Sept. 30, 2015, 12:15 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Finally, calculate schedule latency with the sched trace events, and add it 
> to our statistics
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 
>   src/slave/containerizer/isolators/cgroups/perf_event.hpp 
> 1f722ef3ef7ab7fce5542d4affae961d6cec2406 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 03035dfbfb84470ba39ed9ecfd1eb73890e3f784 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
> 
> Diff: 

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Alexander Rojas]

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

(Updated Feb. 26, 2016, 10:29 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Added comments.


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


Repository: mesos


Description
---

Under certains circumstances, the future returned by poll is discarded right
after the event is triggered, this causes the event callback to be called
before the discard callback which results in an abort signal being raised
by the libevent library.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_poll.cpp 
461624ca003e97be5ea9cf956d86fc294e6f1bc1 

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


Testing
---

```bash
# On CentOS 6.7 running in virtualbox
../configure --enable-ssl --enable-libevent
make -j4 check
sudo ./bin/mesos-tests.sh 
--gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
--gtest_repeat=1000
```


Thanks,

Alexander Rojas

Re: Review Request 43904: Windows: Removed `rootfs` launcher flag, preventing `chroot`.

[Alex Naparu]

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


Ship it!




Ship It!

- Alex Naparu


On Feb. 25, 2016, 7:17 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43904/
> ---
> 
> (Updated Feb. 25, 2016, 7:17 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4780
> https://issues.apache.org/jira/browse/MESOS-4780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `chroot` is does not exist on Windows. Unfortunately, the launcher also
> depends on it. In this commit, we remove Windows support for the
> launcher flag `rootfs`, which controls whether we use `chroot` in the
> launcher. This allows us to divest ourselves of `chroot` altogether on
> Windows.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 129406abdff715e321f683911e404c46676b6daf 
>   src/slave/containerizer/mesos/launch.hpp 
> 7e29ca2b8bec1c20aef122472cff60f6003603ad 
>   src/slave/containerizer/mesos/launch.cpp 
> 6b3bf163e2a577e6318a4a62f96d6bfd98ef9ae9 
> 
> Diff: https://reviews.apache.org/r/43904/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43905: Windows: Removed `user` launcher flag, preventing `su`.

[Alex Naparu]

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


Ship it!




Ship It!

- Alex Naparu


On Feb. 25, 2016, 7:17 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43905/
> ---
> 
> (Updated Feb. 25, 2016, 7:17 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `su` does not exist on Windows. Unfortunately, the launcher also depends
> on it. In this commit, we remove Windows support for the launcher flag
> `user`, which controls whether we use `su` in the launcher. This
> allows us to divest ourselves of `su` altogether on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> d917a99a46841156dc1e359c44010938cc45e943 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 129406abdff715e321f683911e404c46676b6daf 
>   src/slave/containerizer/mesos/launch.hpp 
> 7e29ca2b8bec1c20aef122472cff60f6003603ad 
>   src/slave/containerizer/mesos/launch.cpp 
> 6b3bf163e2a577e6318a4a62f96d6bfd98ef9ae9 
> 
> Diff: https://reviews.apache.org/r/43905/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43903: Stout: Add `WindowsError` constructor to `Result`.

[Alex Naparu]

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


Ship it!




Ship It!

- Alex Naparu


On Feb. 25, 2016, 7:13 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43903/
> ---
> 
> (Updated Feb. 25, 2016, 7:13 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Add `WindowsError` constructor to `Result`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp 
> 577c8e459f505b7b9021153d5e42e9d5a892ec33 
> 
> Diff: https://reviews.apache.org/r/43903/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43416: Windows: Removed ambiguous call to `::write`.

[Alex Clemmer]

> On Feb. 25, 2016, 12:22 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp, line 52
> > 
> >
> > Why is the cast to `size_t` needed here?
> 
> Alex Clemmer wrote:
> It is required, and (though it has been awhile) I believe it's because on 
> 64-bit machines, without this type information, 1 could be one of several 
> integral types. The cast disambiguates.
> 
> Michael Park wrote:
> I only see one definition of `::write` in 
> `3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp`, which has the 
> signature: `inline auto write(int fd, const void* buffer, size_t count)`.
> 
> Doesn't seem like there should be any ambiguity here, are there other 
> defintions of `write` that we're contending with? Because `1` is clearly 
> convertible to `size_t`.
> 
> Alex Clemmer wrote:
> There is also `::write` in the Windows implementation of the C Runtime. 
> To confirm that this is the problem, we can delete the `inline auto write` 
> you mention, and the compile problem disappears. (I just confirmed this is 
> true.)
> 
> So, practically speaking, there seem to be two choices:
> 
> (1) Delete the `::write` in windows.hpp.
> (2) Add a cast here.
> 
> Note that if we choose (1) we should probably then fork every file where 
> where we call `::write` in the code, and replace those calls with `::_write`, 
> as the former is deprecated.

I added a comment to the review for posterity.


- Alex


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


On Feb. 26, 2016, 9:23 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43416/
> ---
> 
> (Updated Feb. 26, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Removed ambiguous call to `::write`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
> 88b355e09f76f0412c74ad69556572f0079deb8f 
> 
> Diff: https://reviews.apache.org/r/43416/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 43416: Windows: Removed ambiguous call to `::write`.

[Alex Clemmer]

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

(Updated Feb. 26, 2016, 9:23 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Windows: Removed ambiguous call to `::write`.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
88b355e09f76f0412c74ad69556572f0079deb8f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42733]

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

- Mesos ReviewBot


On Feb. 26, 2016, 6:31 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42733/
> ---
> 
> (Updated Feb. 26, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos, Michael Park, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-3940
> https://issues.apache.org/jira/browse/MESOS-3940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checks for presence of `ReservationInfo.principal`.
> 
> The `ReservationInfo.principal` field was recently made `optional`. There 
> remain a couple places in the code that assume this field is always set. This 
> patch uses `has_principal()` before this field is accessed to ensure safety.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 
> 
> Diff: https://reviews.apache.org/r/42733/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43416: Windows: Removed ambiguous call to `::write`.

[Alex Clemmer]

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

(Updated Feb. 26, 2016, 9:21 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Windows: Removed ambiguous call to `::write`.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
88b355e09f76f0412c74ad69556572f0079deb8f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 43796: Added documentation for `cgroups/net_cls` isolator.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On Feb. 22, 2016, 6:26 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43796/
> ---
> 
> (Updated Feb. 22, 2016, 6:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Bugs: MESOS-4660
> https://issues.apache.org/jira/browse/MESOS-4660
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for `cgroups/net_cls` isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 87f145cd957dcb8fd3188c866212b417f0ab6296 
> 
> Diff: https://reviews.apache.org/r/43796/diff/
> 
> 
> Testing
> ---
> 
> Built the web-site using docker, and proof read the website and links on 
> localhost.
> 
> Verified all the links embedded in markdown work.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43662: Added support for pipelining calls to the scheduler library.

[Vinod Kone]

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




src/scheduler/scheduler.cpp (lines 203 - 206)


kill this and do all the drops after #213.



src/scheduler/scheduler.cpp (line 214)


//.
if (call.type() == SUBSCRIBE && state != CONNECTED) {
  drop();
  return;
}

//.
if (call.type() != SUBSCRIBE && state != SUBSCRIBED) {
  drop();
  return;
}



src/scheduler/scheduler.cpp (lines 232 - 239)


Kill this.



src/scheduler/scheduler.cpp (lines 233 - 239)


It's better to consolidate all drops before #215.



src/scheduler/scheduler.cpp (line 246)


shouldn't we drop calls if we are not in SUBSCRIBED state? see above.



src/scheduler/scheduler.cpp (line 249)


CHECK_SOME(connectionId); ?

also, why is this method taking an Option instead of 
ConnectionID?



src/scheduler/scheduler.cpp (line 276)


why is this taking an option?



src/scheduler/scheduler.cpp (line 290)


Add a VLOG(1)?



src/scheduler/scheduler.cpp (line 378)


what if we are in CONNECTING state?



src/scheduler/scheduler.cpp (line 387)


s/old//



src/scheduler/scheduler.cpp (line 457)


what about other states? can we be in CONNECTING here for example?



src/scheduler/scheduler.cpp (line 498)


// We reset the state to CONNECTED if...



src/scheduler/scheduler.cpp (line 533)


Do we need 'subscribed' as a member variable now? Can we not detect this 
via state and/or connectionID?



src/scheduler/scheduler.cpp (line 546)


Can you add a comment here on when this can happen?



src/scheduler/scheduler.cpp (line 567)


what about other states?



src/scheduler/scheduler.cpp (line 609)


s/connection/connections/



src/scheduler/scheduler.cpp (line 610)


s/connection/connections/


- Vinod Kone


On Feb. 25, 2016, 4:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43662/
> ---
> 
> (Updated Feb. 25, 2016, 4:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
> https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the scheduler library used to chain calls on previous call 
> responses. This was inherently slow. This change adds support for pipelining 
> all calls to the master on a single connection via the `http::Connection` 
> abstraction in libprocess.
> 
> This change also adds support for handling various error scenarios when we 
> notice a disconnection instead of just relying on the master detector for 
> invoking the `disconnected` callback.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
> 
> Diff: https://reviews.apache.org/r/43662/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43960: Added an end-to-end test for docker registry puller.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Feb. 26, 2016, 9:29 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43960/
> ---
> 
> (Updated Feb. 26, 2016, 9:29 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an end-to-end test for docker registry puller.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> b3c6f8848f2ceb2bd4dce35d6e7f813c2d4d2bd9 
> 
> Diff: https://reviews.apache.org/r/43960/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

[Joseph Wu]

> On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 260
> > 
> >
> > All this is nice to know, but ho does it connect to the variable 
> > "isShutdown" and why is it named this way? What does "isShutdown == true" 
> > signify? This should be in the comment IMHO. Or maybe a better variableName 
> > could make this more easy to grasp? Suggestion:
> > 
> > bool cleanUpContainersInDestructor = true

I've reworked this group of associated comments; and renamed the bool while I 
was at it.

I think its clearer now (and more verbose about why it exists).


> On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 264
> > 
> >
> > This global var makes the code hard to follow.
> > 
> > And I am quite confused by the source code comment.
> > 
> > Which is the case here?
> > 
> > A. The tests in question execute shutdown logic themselves. So they 
> > still need the containerizer to be destroyed. But that's what isShutdown 
> > switches on and off futher down below, not the shutdown logic. 
> > 
> > B. The tests in question want the shutdown logic to be executed by 
> > "terminate()" but not the containerizer destruction. This is what setting 
> > isShutdown to true seems to cause. But the tests where I see terminate() in 
> > use sometimes need the containerizer destroyed, because they create a new 
> > one right away. And sometimes they do not.
> > 
> > What am I missing?

A) The tests execute shutdown logic themselves.  They want to destroy the 
containerizer, **but not the containers**.  `isShutdown` only determines 
whether we destroy containers.  The containerizer's destructor does not destroy 
containers.

B) Tests will do `slave->terminate()` to shutdown the slave.  They need to do 
`slave.reset()` to call destructors on all the slave's dependencies.
Tests need to recreate the containerizer because you cannot use the same 
containerizer instance over two slaves (i.e. you can't `containerizer->recover` 
twice).
Tests that don't recreate the containerizer all use the `TestContainerizer`, 
which is just a mock anyway.

(I'll leave this issue open so that you can close, if the new comments + my 
notes are sufficient.)


> On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 504
> > 
> >
> > Should this be placed after the cointainerizer destruction maybe? Say 
> > line 399.
> > 
> > But then I still do not get to observe a consistent use pattern of 
> > terminate(). See my comment above.

As noted above, the container**izer** isn't destructed here.  The *containers* 
are.


- Joseph


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


On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated Feb. 26, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

[Joseph Wu]

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

(Updated Feb. 26, 2016, 11:50 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, 
and Michael Park.


Changes
---

Reworked some comments and renamed `isShutdown`.


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


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43782: Changed object of `CreateVolume` ACL to `roles`.

[Greg Mann]

> On Feb. 26, 2016, 7:33 p.m., Jie Yu wrote:
> > src/master/master.cpp, line 2934
> > 
> >
> > Ditto on using 'contains'

I think I probably pushed an update while you were reviewing, this should be in 
the latest diff :-) Thx!


- Greg


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


On Feb. 26, 2016, 7:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43782/
> ---
> 
> (Updated Feb. 26, 2016, 7:08 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed object of `CreateVolume` ACL to `roles`.
> 
> This solves a problem in which any principal could create volumes for any 
> role using the '/create-volumes' operator endpoint.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 
>   src/authorizer/local/authorizer.cpp 
> 9557bbdf68ff182c4538bbf70cee576d717abc05 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/tests/authorization_tests.cpp 9d046e8d53cbb6c065a23ca3f7832021ec7faadc 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 6069ca1e9ed278459c5182e438417e95955b1924 
>   src/tests/persistent_volume_tests.cpp 
> e169e1b141a38dc389eefd42c11a078c413123d5 
> 
> Diff: https://reviews.apache.org/r/43782/diff/
> 
> 
> Testing
> ---
> 
> Persistent volume tests and operation validation tests were altered to 
> accomodate the new ACL object, and some new principal/role combinations were 
> added to `AuthorizationTest.CreateVolume`.
> 
> Ran `configure && make check` and `configure --enable-libevent --enable-ssl 
> && make check` on OSX; all tests passed.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43800: Updated docs for reservation, volumes, and authZ.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 26, 2016, 7:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43800/
> ---
> 
> (Updated Feb. 26, 2016, 7:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docs for reservation, volumes, and authZ.
> 
> This updates the authorization documentation to include the new `roles` 
> object for the `CreateVolume` and `ReserveResources` ACLs. The docs for 
> persistent volumes and dynamic reservations are also updated to reflect the 
> new authorization behavior. A note has been added to `upgrades.md` detailing 
> the impact of these changes on upgrades.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md bbb4f2adc9348cb1686e6af78f5604d8cf7651ab 
>   docs/persistent-volume.md 2a794a572ff930aa1f95706b89fef9243be627de 
>   docs/reservation.md b98ebe6df0739b48c5fa58e087fd64b1c6c5d456 
>   docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 
> 
> Diff: https://reviews.apache.org/r/43800/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43798: Added overview section to upgrades.md.

[Joerg Schad]

> On Feb. 26, 2016, 7:02 p.m., Neil Conway wrote:
> > Basic impression: this isn't ideal, but I'm not sure we can do better given 
> > the formatting constraints of Markdown. So, ship it I guess? :)

As discussed earlier I agree with you in both aspects


- Joerg


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


On Feb. 23, 2016, 9:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43798/
> ---
> 
> (Updated Feb. 23, 2016, 9:39 p.m.)
> 
> 
> Review request for mesos, Michael Park, Neil Conway, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4381
> https://issues.apache.org/jira/browse/MESOS-4381
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added overview section to upgrades.md.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 
> 
> Diff: https://reviews.apache.org/r/43798/diff/
> 
> 
> Testing
> ---
> 
> Viewed via gist (https://gist.github.com/joerg84/eddbc0302a5a4b291e81) and 
> docker website container.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 43779: Added '/reserve' tests with multiple roles.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 26, 2016, 4:58 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43779/
> ---
> 
> (Updated Feb. 26, 2016, 4:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added '/reserve' tests with multiple roles.
> 
> Operators may reserve resources for multiple roles in the same operation; 
> this patch adds tests to confirm correct behavior of authorization in this 
> case. The tests `ReservationEndpointsTest.GoodReserveACLMultipleRoles` and 
> `ReservationEndpointsTest.BadReserveACLMultipleRoles` were added.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> 32b2af4115211b58a5127a14dd19152c2eca120c 
> 
> Diff: https://reviews.apache.org/r/43779/diff/
> 
> 
> Testing
> ---
> 
> Ran `configure && make check` and `configure --enable-libevent --enable-ssl 
> && make check` on OSX; all tests passed.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43960: Added an end-to-end test for docker registry puller.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43958, 43959, 43960]

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

- Mesos ReviewBot


On Feb. 26, 2016, 5:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43960/
> ---
> 
> (Updated Feb. 26, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an end-to-end test for docker registry puller.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> b3c6f8848f2ceb2bd4dce35d6e7f813c2d4d2bd9 
> 
> Diff: https://reviews.apache.org/r/43960/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 43778: Added '/create-volumes' tests with multiple roles.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 26, 2016, 4:56 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43778/
> ---
> 
> (Updated Feb. 26, 2016, 4:56 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added '/create-volumes' tests with multiple roles.
> 
> Operators may create volumes for multiple roles in the same operation; this 
> patch adds tests to confirm correct behavior of authorization in this case. 
> The tests `ReservationEndpointsTest.GoodReserveACLMultipleRoles` and 
> `ReservationEndpointsTest.BadReserveACLMultipleRoles` were added.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 6069ca1e9ed278459c5182e438417e95955b1924 
> 
> Diff: https://reviews.apache.org/r/43778/diff/
> 
> 
> Testing
> ---
> 
> Ran `configure && make check` and `configure --enable-libevent --enable-ssl 
> && make check` on OSX; all tests passed.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Joris Van Remoortere]

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




3rdparty/libprocess/src/libevent_poll.cpp (line 50)


Please add a very clear comment here that calls out the effect of this 
destructor invoking the event_free, which implies `event_del` which means the 
event will no longer be `pending`, and that's why we won't execute a 2nd time



3rdparty/libprocess/src/libevent_poll.cpp (lines 61 - 63)


can you add a comment explaining why we are testing `event_pending`?
Another comment for explaining that `event_active` triggers pollCallback to 
be run in the `event_loop`



3rdparty/libprocess/src/libevent_poll.cpp (lines 83 - 85)


let's call out here that we're binding in the destructor, and the 
consequences of that.



3rdparty/libprocess/src/libevent_poll.cpp (lines 90 - 91)


Please comment why we're using a weak_ptr here.



3rdparty/libprocess/src/libevent_poll.cpp (lines 91 - 93)


Please comment that this is the explicit order we want these 2 statements 
in.


- Joris Van Remoortere


On Feb. 24, 2016, 9:30 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 24, 2016, 9:30 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43782: Changed object of `CreateVolume` ACL to `roles`.

[Jie Yu]

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


Fix it, then Ship it!





src/master/master.cpp (line 2934)


Ditto on using 'contains'


- Jie Yu


On Feb. 26, 2016, 7:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43782/
> ---
> 
> (Updated Feb. 26, 2016, 7:08 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4591
> https://issues.apache.org/jira/browse/MESOS-4591
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed object of `CreateVolume` ACL to `roles`.
> 
> This solves a problem in which any principal could create volumes for any 
> role using the '/create-volumes' operator endpoint.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 
>   src/authorizer/local/authorizer.cpp 
> 9557bbdf68ff182c4538bbf70cee576d717abc05 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/tests/authorization_tests.cpp 9d046e8d53cbb6c065a23ca3f7832021ec7faadc 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 6069ca1e9ed278459c5182e438417e95955b1924 
>   src/tests/persistent_volume_tests.cpp 
> e169e1b141a38dc389eefd42c11a078c413123d5 
> 
> Diff: https://reviews.apache.org/r/43782/diff/
> 
> 
> Testing
> ---
> 
> Persistent volume tests and operation validation tests were altered to 
> accomodate the new ACL object, and some new principal/role combinations were 
> added to `AuthorizationTest.CreateVolume`.
> 
> Ran `configure && make check` and `configure --enable-libevent --enable-ssl 
> && make check` on OSX; all tests passed.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44073: Disabled metrics endpoint rate limiting in mesos tests.

[Alexander Rojas]

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




src/tests/main.cpp (line 75)


I think this is using the posix `setenv(const char*, const char*, int)`, 
though the boolean as last parameter makes me doubt.

I'd recommend using our own `os::setenv(std::string, std::string, bool)`


- Alexander Rojas


On Feb. 26, 2016, 5:12 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44073/
> ---
> 
> (Updated Feb. 26, 2016, 5:12 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4783
> https://issues.apache.org/jira/browse/MESOS-4783
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Disabled metrics endpoint rate limiting in mesos tests.
> 
> 
> Diffs
> -
> 
>   src/tests/main.cpp 942488e57419ace8b7a821f53024aced0f43c7d9 
> 
> Diff: https://reviews.apache.org/r/44073/diff/
> 
> 
> Testing
> ---
> 
> make distcheck (ubuntu 12 & 14), make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44071: Allowed disabling metrics endpoint rate limiting via the environment.

[Alexander Rojas]

> On Feb. 26, 2016, 6 p.m., Benjamin Bannier wrote:
> > docs/configuration.md, line 1690
> > 
> >
> > Rename to `LIBPROCESS_METRICS_RATE_LIMIT` and document changed 
> > semantics (comment in previous patch).

Does this mean that rate limiting will be opt-in?

I wonder how that will affect production systems.


- Alexander


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


On Feb. 26, 2016, 4:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44071/
> ---
> 
> (Updated Feb. 26, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4776
> https://issues.apache.org/jira/browse/MESOS-4776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed disabling metrics endpoint rate limiting via the environment.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 
> 
> Diff: https://reviews.apache.org/r/44071/diff/
> 
> 
> Testing
> ---
> 
> Site rendered with packaged docker container.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>