Re: Review Request 39056: Fix for Mesos master crash due to check failure.

[Mandeep Chadha]

> On Oct. 7, 2015, 1:03 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 879
> > 
> >
> > The meos is now using 0.01 as the MIN_CPUS
> 
> Guangya Liu wrote:
> As the mesos is using 0.01 as the MIN_CPUS, I think it is OK using 0.01 
> as the epsilon

Are we ok with changes ? Friendly ping :)


- Mandeep


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


On Oct. 6, 2015, 10:07 p.m., Mandeep Chadha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39056/
> ---
> 
> (Updated Oct. 6, 2015, 10:07 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check failed due to double comparison : MESOS-3552.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39056/diff/
> 
> 
> Testing
> ---
> 
> Added unit test.
> make check successful.
> 
> 
> Thanks,
> 
> Mandeep Chadha
> 
>

Re: Review Request 39886: Added documentation about roles.

[Alexander Rukletsov]

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



docs/roles.md (line 10)


Roles also introduce one level in the hierarchy of resource allocation. 
Also putting a framework into a role is the only way to leverage some sort of 
prioritization (role weights). Similar to what you say at the bottom of the 
doc, I would say it makes sense to mention it here as a use case as well.



docs/roles.md (line 12)


Do you want to be consistent with the existing codebase and continue use 
"slave" for now?



docs/roles.md (line 22)


Shall we leave a note here that our intention is to add dynamic roles ASAP?


- Alexander Rukletsov


On Nov. 3, 2015, 1:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 3, 2015, 1:51 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/configuration.md e66013ad0b951049df645ff90907f19ae8081cc2 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 39857: Master/Slave: Add a REASON for TASK_LOST with hard slave drain

[Guangya Liu]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

Update main logic in mesos for both master and slave.


Diffs
-

  src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
  src/master/master.cpp 9f4586e668a2141f4937497d42853fbdea7751a5 
  src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 38214: PROTO: Add a REASON for TASK_LOST with hard slave drain

[Guangya Liu]

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

(Updated 十一月 3, 2015, 8:21 a.m.)


Review request for mesos and Ben Mahler.


Summary (updated)
-

PROTO: Add a REASON for TASK_LOST with hard slave drain


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


Repository: mesos


Description
---

Ensure that TASK_LOST for a hard slave drain (SIGUSR1) include a
reason.


Diffs (updated)
-

  include/mesos/mesos.proto 94004343ea615d87d7c7d63a6a811cf8002e 
  include/mesos/v1/mesos.proto 8131778fe5c5f3a47ae9300a811e3d857a22da6a 
  src/messages/messages.proto 9a1564ff38fa1b984e92cb0abfde2108385f9e2a 

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


Testing (updated)
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 39060: Create master detector per url & not per framework

[Mandeep Chadha]

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

(Updated Nov. 3, 2015, 9:32 a.m.)


Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
---

If the number of framework created exceeds the lib process
threads then during master failover the zookeeper updates can
cause deadlock.


Diffs (updated)
-

  include/mesos/scheduler.hpp f571d42d1508632152473c4f4ade60ae3900fce1 
  src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 

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


Testing
---

make check successful 
Created 100 framework instances on a 24 CPU machine. Master failover detected 
by the framework process and continue to work as expected.


Thanks,

Mandeep Chadha

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

> On Oct. 25, 2015, 8:31 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, line 564
> > 
> >
> > We should unify if use "role1" or role1 for all of the tests

I vote for variables, since they were introduced exactly for this.


> On Oct. 25, 2015, 8:31 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, line 433
> > 
> >
> > s/Set_InvalidRequest/SetInvalidRequest?
> > 
> > Ditto for the following

I think the intention was to indicate these tests relate to quota set requests. 
However, I'm +1 for removing the underscore, because it violates the 
consistency.


- Alexander


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


On Oct. 23, 2015, 9:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Oct. 23, 2015, 9:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

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


Great tests, thanks a lot Jörg!


src/tests/master_quota_tests.cpp (lines 156 - 163)


It looks like you have implemented most of those. Mind updating this 
`TODO`? Thanks!



src/tests/master_quota_tests.cpp (line 429)


We already have a section for validation requests, I'd suggest you move 
those there : ). I'd say, around L197, after `NonExistentRole`.



src/tests/master_quota_tests.cpp (line 431)


It looks like this comment is a victim of partial refactoring : ). Could 
you please udpate it?

Also we usually use 3rd person in test comments.

```
// This test ensures...
```
or
```
// This tests that...
```
or simply
```
// Tests whether...
```



src/tests/master_quota_tests.cpp (line 433)


I think what Jörg tries to express is that these tests are for "set-path" 
of quota. However it indeed violates consistency, hence I'm +1 for removing the 
underscore.



src/tests/master_quota_tests.cpp (lines 442 - 444)


`clang-format` suggests the following:
```
  string badRequest =
"{"
"invalidJson"
"}";
```



src/tests/master_quota_tests.cpp (line 450)


You can avoid wrapping `badRequest` in `Option<>`.



src/tests/master_quota_tests.cpp (line 451)


Do you need `None()` here? I think you can omit it.



src/tests/master_quota_tests.cpp (line 453)


You don't need a fully qualified type name here thanks to `using` 
directives above. I would also suggest to print additional info in case of 
failure. Putting all together, how about
```
AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response)
<< response.get().body;
```
?

The suggestion holds for tests below as well.



src/tests/master_quota_tests.cpp (line 461)


Same here: 3rd person singular.



src/tests/master_quota_tests.cpp (line 463)


We try to keep tests atomic: testing one thing at a time. However, I think 
it does make sense to merge `Set_InvalidJson`, `Set_InvalidJson2`, and 
`Set_InvalidResources` into one tests for brevity and speed. These 3 tests 
address the same issue: malformed JSON, similar to what you have done with 
`Set_InvalidResourceInfos`.



src/tests/master_quota_tests.cpp (lines 472 - 474)


Formatting, see above.



src/tests/master_quota_tests.cpp (lines 480 - 481)


See suggestion for `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 483)


See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 491)


3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 493)


See suggestion above, does it make sense to meld this test into previous 
ones?



src/tests/master_quota_tests.cpp (line 513)


See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 521)


3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 522)


Why capitalize "Code"? Here and below.



src/tests/master_quota_tests.cpp (lines 536 - 540)


Correct indentation.



src/tests/master_quota_tests.cpp (line 542)


See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 550)


3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 552)


How about renaming the test to `SetMultipleRoles`?



src/tests/master_quota_tests.cpp (lines 563 - 564)


This fits one line.



src/tests/master_quota_tests.cpp (line 564)

Re: Review Request 36913: Added /quota HTTP Endpoint for Quota handling.

[Joerg Schad]

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

(Updated Nov. 3, 2015, 1:31 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Alex Clemmer, 
and Joris Van Remoortere.


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


Repository: mesos


Description
---

Added /quota HTTP Endpoint for Quota handling.


Diffs (updated)
-

  src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 
  src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
  src/master/http.cpp 093f79384916dc08b32b70d3614e0ff314825c42 
  src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
  src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39060: Create master detector per url & not per framework

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39060]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 9:32 a.m., Mandeep Chadha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39060/
> ---
> 
> (Updated Nov. 3, 2015, 9:32 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3595
> https://issues.apache.org/jira/browse/MESOS-3595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the number of framework created exceeds the lib process
> threads then during master failover the zookeeper updates can
> cause deadlock.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp f571d42d1508632152473c4f4ade60ae3900fce1 
>   src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 
> 
> Diff: https://reviews.apache.org/r/39060/diff/
> 
> 
> Testing
> ---
> 
> make check successful 
> Created 100 framework instances on a 24 CPU machine. Master failover detected 
> by the framework process and continue to work as expected.
> 
> 
> Thanks,
> 
> Mandeep Chadha
> 
>

Re: Review Request 39886: Added documentation about roles.

[Adam B]

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


Great start. I agree with AlexR that the hierarchical DRF use of roles is 
important enough to mention at the beginning alongside reservations.


docs/roles.md (lines 31 - 32)


As an administrator, you can set `--acls` to declare which framework 
principals can register as which roles.



docs/roles.md (lines 52 - 53)


Unless the slave is started with `--default_role="foo"`, in which case all 
resources on that slave are initially reserved for the `foo` role.



docs/roles.md (line 56)


Also note that you cannot create a persistent volume on unreserved 
resources (those with the `*` role). You must first reserve the disk resource 
and then create the volume.



docs/roles.md (line 61)


I would call this out at the beginning too, since reservations and the 
hierarchical allocation strategy are the two primary functions of roles in 
Mesos (assuming the default allocator).



docs/roles.md (lines 64 - 65)


Also worth mentioning `--weights` here since that affects the current/fair 
share calculation.


- Adam B


On Nov. 2, 2015, 5:51 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 2, 2015, 5:51 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/configuration.md e66013ad0b951049df645ff90907f19ae8081cc2 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39401: Quota: Updated allocate() in the hierarchical allocator to support quota.

[Qian Zhang]

> On Nov. 1, 2015, 8:14 p.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1005
> > 
> >
> > For this TODO, what do we plan to do in future? Include the dynamic 
> > reserved resources for this role on this agent in 
> > ```roleConsumedResources```? And what about the static reserved resources?
> 
> Alexander Rukletsov wrote:
> Dynamic reservations should account towards role's quota. [Note about 
> static 
> reservations](https://docs.google.com/document/d/16iRNmziasEjVOblYp5bbkeBZ7pnjNlaIzPQqMTHQ-9I/edit?pli=1#heading=h.xumvch9xiky2)

So when we check if a role's quota is satisfied or not, we will add the role's 
allocated resources with the role's dynamically reserved resources, and check 
if the sum contains the role's quota. But for role's statically reserved 
resources, we will consider they are part of quota. So in future (after these 
TODOs are handled) when we check if a role's quota is satified or not, the 
formula should be ```(role's allocated resource + role's dynamically reserved 
resources) > (role's quota + role's statically reserved resources)```, right?


- Qian


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


On Nov. 2, 2015, 11:16 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> ---
> 
> (Updated Nov. 2, 2015, 11:16 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
> 
> Diff: https://reviews.apache.org/r/39401/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38110: Quota: Checked sanity of quota set requests.

[Alexander Rukletsov]

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

(Updated Nov. 3, 2015, 3:46 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Michael Park.


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


Repository: mesos


Description
---

Performs a check whether a quota request is reasonable and can be satisfied at 
the moment. A precise answer is impossible here, but a sanity check is still 
helpful, because it allows us to filter knowingly unsatisfiable requests.


Diffs
-

  src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

(Updated Nov. 3, 2015, 3:46 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs
-

  src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38956: Quota: Added allocator-agnostic tests.

[Alexander Rukletsov]

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

(Updated Nov. 3, 2015, 3:47 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Michael Park.


Summary (updated)
-

Quota: Added allocator-agnostic tests.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/Makefile.am 98cbafc134ec388a176d50172912fbfdf9f5bfa3 
  src/master/quota_handler.cpp PRE-CREATION 
  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 39866: Added containerInfo support for tasks in mesos containerizer.

[Kapil Arya]

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


Can we merge the Tests from https://reviews.apache.org/r/39769/?

- Kapil Arya


On Nov. 2, 2015, 1:59 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39866/
> ---
> 
> (Updated Nov. 2, 2015, 1:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added containerInfo support for tasks in mesos containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4aad8a3be43b331efc6b8157b2fae090df16c1b4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9fd69c1738e2300dbb843d259727010e24523cff 
> 
> Diff: https://reviews.apache.org/r/39866/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39325: Fixed race between coordinator election and recovery in replicated log.

[Jie Yu]

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

Ship it!


Thanks! LGTM!


src/log/consensus.cpp (line 46)


`static bool isRejectedPromise(...)`

This is a file local helper. Let's make it explicit.



src/log/consensus.cpp (line 57)


Ditto here.



src/tests/log_tests.cpp (lines 718 - 719)


We try to avoid jagness like this. The following might be better:

```
Future recovering1 =
  recover(2, Owned(replica1), network, true);
```


- Jie Yu


On Nov. 2, 2015, 7:42 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39325/
> ---
> 
> (Updated Nov. 2, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Bugs: MESOS-3280
> https://issues.apache.org/jira/browse/MESOS-3280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3280. The basic problem is that replicas silently ignore inbound Promise
> and Write requests if they have not finished the recovery protocol yet 
> (because
> they can't safely vote on such requests). Hence, if we try to do a Paxos round
> while a quorum of nodes have not finished recovering, the Paxos round will 
> never
> complete. In particular, this might happen during coordinator election:
> coordinator election (which is implemented as performing a full Paxos round)
> starts as soon as the candidate coordinator replica has finished the recovery
> protocol. If several nodes start concurrently, a quorum of those nodes might
> still be executing the recovery protocol, and hence the coordinator will never
> be elected.
> 
> To address this, add "ignored" responses to the Promise and Write 
> sub-protocols:
> if a proposer sees a quorum of "ignored" responses to a promise or write 
> request
> it has issued, it knows the request will never succeed.  When used for
> coordinator election, the current coding will retry immediately (without a
> backoff).
> 
> Note that replicas will still silently drop promise/write requests if another
> kind of problem occurs (e.g., an I/O error prevents reading/writing log
> data). We might consider changing this, although it will require some thought:
> e.g., if a replica's disk is broken, sending an "ignored" message on every
> request might flood the network.
> 
> REVIEWER NOTES:
> 
> * GMock code is ugly, but see notes below.
> * Should we add exponential backoff when retrying coordinator election?
> 
> 
> Diffs
> -
> 
>   src/log/consensus.cpp 71cd5f39c02f583c1ea53d1ab4569115b0cee2a3 
>   src/log/coordinator.cpp e1df8b01f28447955e1e8bbd764fce3e1a948d88 
>   src/log/replica.hpp 70f415f4f81465dbb7d1e4d4f65c7965cd432415 
>   src/log/replica.cpp 414e1165ae969c1f9492e7b8361c72df2c4fbc16 
>   src/messages/log.proto d73b33f865963292af580945659ad0e800f2a204 
>   src/tests/log_tests.cpp 222a12e10032082e6315ca7b43d3393738edd312 
> 
> Diff: https://reviews.apache.org/r/39325/diff/
> 
> 
> Testing
> ---
> 
> "make check" passes, including a new test that uses a newly constructed mock 
> to ensure we're testing the message schedule described above.
> 
> I also wrote a script stops and starts mesos-master in a loop, removing the 
> replicated log each time. Without the patch, this occasionally fails with a 
> "registry fetch" timeout; with the patch, you can observe several scenarios 
> where coordinator election is reborted and retried because a quorum of 
> ignored responses is seen. Note that in some cases, we need to retry 
> coordinator election up to ~70 times (!), because we don't currently use a 
> backoff. On a more realistic LAN configuration, many fewer retries will 
> likely be needed, but we could also use a backoff instead. But the important 
> point is that election eventually succeeds and we don't hang.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Neil Conway]

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



docs/upgrades.md (line 16)


AFAIK this isn't Java-specific.


- Neil Conway


On Nov. 3, 2015, 6 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 6 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39060: Create master detector per url & not per framework

[Mandeep Chadha]

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

(Updated Nov. 3, 2015, 6:06 p.m.)


Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
---

If the number of framework created exceeds the lib process
threads then during master failover the zookeeper updates can
cause deadlock.


Diffs (updated)
-

  include/mesos/scheduler.hpp f571d42d1508632152473c4f4ade60ae3900fce1 
  src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 

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


Testing
---

make check successful 
Created 100 framework instances on a 24 CPU machine. Master failover detected 
by the framework process and continue to work as expected.


Thanks,

Mandeep Chadha

Re: Review Request 39908: Added a section to Upgrades doc

[Michael Park]

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

Ship it!


Modulo Neil's comment.

- Michael Park


On Nov. 3, 2015, 6 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 6 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Review Request 39910: Fix build for RegistryClientTest.SimpleGetManifest

[Joseph Wu]

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

Review request for mesos, Ben Mahler and Artem Harutyunyan.


Repository: mesos


Description
---

These variables were renamed these commits:

* 
https://github.com/apache/mesos/commit/f556d24f345a8b4a484ff4affe65a92d76383380
* 
https://github.com/apache/mesos/commit/92a0c16a1518a075ef8c5302b2f9637ff2f2a01a


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
99c2af7d06dd7908e8d77e1bfb3e20622fb3eb4c 

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


Testing
---

`configure --enable-libevent --enable-ssl`
`make check`


Thanks,

Joseph Wu

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

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

(Updated Nov. 3, 2015, 6 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Added mention to Credential protobuf change in 0.26 in uprades.md


Diffs
-

  docs/upgrades.md 3854e59 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

> On Nov. 3, 2015, 7:21 p.m., Neil Conway wrote:
> > docs/upgrades.md, line 16
> > 
> >
> > AFAIK this isn't Java-specific.

This will mostly impact Java frameworks since they'll have to not only update 
the generated protos but change their framework code. See: 
https://github.com/apache/mesos/commit/38dbadc944f56e24725fba102bbd2db76cb31228#diff-2ca9e1a2a6f66a790bbbfae40708ecfaL249


- Isabel


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


On Nov. 3, 2015, 6 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 6 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39908]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 6 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 6 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39325: Fixed race between coordinator election and recovery in replicated log.

[Neil Conway]

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

(Updated Nov. 3, 2015, 7:13 p.m.)


Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen.


Changes
---

Address code review comments.


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


Repository: mesos


Description
---

MESOS-3280. The basic problem is that replicas silently ignore inbound Promise
and Write requests if they have not finished the recovery protocol yet (because
they can't safely vote on such requests). Hence, if we try to do a Paxos round
while a quorum of nodes have not finished recovering, the Paxos round will never
complete. In particular, this might happen during coordinator election:
coordinator election (which is implemented as performing a full Paxos round)
starts as soon as the candidate coordinator replica has finished the recovery
protocol. If several nodes start concurrently, a quorum of those nodes might
still be executing the recovery protocol, and hence the coordinator will never
be elected.

To address this, add "ignored" responses to the Promise and Write sub-protocols:
if a proposer sees a quorum of "ignored" responses to a promise or write request
it has issued, it knows the request will never succeed.  When used for
coordinator election, the current coding will retry immediately (without a
backoff).

Note that replicas will still silently drop promise/write requests if another
kind of problem occurs (e.g., an I/O error prevents reading/writing log
data). We might consider changing this, although it will require some thought:
e.g., if a replica's disk is broken, sending an "ignored" message on every
request might flood the network.

REVIEWER NOTES:

* GMock code is ugly, but see notes below.
* Should we add exponential backoff when retrying coordinator election?


Diffs (updated)
-

  src/log/consensus.cpp 71cd5f39c02f583c1ea53d1ab4569115b0cee2a3 
  src/log/coordinator.cpp e1df8b01f28447955e1e8bbd764fce3e1a948d88 
  src/log/replica.hpp 70f415f4f81465dbb7d1e4d4f65c7965cd432415 
  src/log/replica.cpp 414e1165ae969c1f9492e7b8361c72df2c4fbc16 
  src/messages/log.proto d73b33f865963292af580945659ad0e800f2a204 
  src/tests/log_tests.cpp 222a12e10032082e6315ca7b43d3393738edd312 

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


Testing
---

"make check" passes, including a new test that uses a newly constructed mock to 
ensure we're testing the message schedule described above.

I also wrote a script stops and starts mesos-master in a loop, removing the 
replicated log each time. Without the patch, this occasionally fails with a 
"registry fetch" timeout; with the patch, you can observe several scenarios 
where coordinator election is reborted and retried because a quorum of ignored 
responses is seen. Note that in some cases, we need to retry coordinator 
election up to ~70 times (!), because we don't currently use a backoff. On a 
more realistic LAN configuration, many fewer retries will likely be needed, but 
we could also use a backoff instead. But the important point is that election 
eventually succeeds and we don't hang.


Thanks,

Neil Conway

Re: Review Request 39910: Fix build for RegistryClientTest.SimpleGetManifest

[Ben Mahler]

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

Ship it!


Ship It!

- Ben Mahler


On Nov. 3, 2015, 6:09 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39910/
> ---
> 
> (Updated Nov. 3, 2015, 6:09 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These variables were renamed these commits:
> 
> * 
> https://github.com/apache/mesos/commit/f556d24f345a8b4a484ff4affe65a92d76383380
> * 
> https://github.com/apache/mesos/commit/92a0c16a1518a075ef8c5302b2f9637ff2f2a01a
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99c2af7d06dd7908e8d77e1bfb3e20622fb3eb4c 
> 
> Diff: https://reviews.apache.org/r/39910/diff/
> 
> 
> Testing
> ---
> 
> `configure --enable-libevent --enable-ssl`
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Oct. 20, 2015, 6:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39449: Documented order of includes.

[Joerg Schad]

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



docs/c++-style-guide.md (line 262)


This should be the first include (see my other comments and the Google 
Styleguide ( 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes) 
:-)).


- Joerg Schad


On Oct. 19, 2015, 9:29 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39449/
> ---
> 
> (Updated Oct. 19, 2015, 9:29 p.m.)
> 
> 
> Review request for mesos, Marco Massenzio and Michael Park.
> 
> 
> Bugs: MESOS-2275
> https://issues.apache.org/jira/browse/MESOS-2275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented order of includes.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 0b6189174a4f0f1815625f68fb1a743b04a9cdad 
> 
> Diff: https://reviews.apache.org/r/39449/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

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

(Updated Nov. 3, 2015, 10:23 p.m.)


Review request for mesos and Michael Park.


Changes
---

Added CHANGELOG section and removed a trailing space


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


Repository: mesos


Description
---

Added mention to Credential protobuf change in 0.26 in uprades.md


Diffs (updated)
-

  CHANGELOG 99d8882 
  docs/upgrades.md 3854e59 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 39018: Added JSON parsing for Resources.

[Greg Mann]

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

(Updated Nov. 3, 2015, 10:55 p.m.)


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


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This includes code changes necessary for JSON parsing of Resources. 
Documentation changes will be posted soon in another review 
(https://reviews.apache.org/r/39102/).


Diffs (updated)
-

  include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
  include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
  src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
  src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
  src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 

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


Testing
---

New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
added: `ResourcesTest.ParsingFromJSONWithRoles` and 
`ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
scenarios that might show up in user-supplied JSON.

`make check` was used to confirm that all tests pass, with one notable failure 
(ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: 
https://issues.apache.org/jira/browse/MESOS-3698

The original resources parsing format is used throughout many other tests 
(check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
original parsing continues to work correctly.


Thanks,

Greg Mann

Re: Review Request 39449: Documented order of includes.

[Joerg Schad]

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

Ship it!



docs/c++-style-guide.md (line 235)


Could we add that for cpp files the repective .h file should be included 
first (which is specified in the Google Styleguide but many files ignore this)



docs/c++-style-guide.md (lines 240 - 241)


Add foo.hpp as first include.



docs/c++-style-guide.md (line 251)


Could we add a short comment above every new section describing the 
representative meaning of each? (e.g. here nested subfolder)


- Joerg Schad


On Oct. 19, 2015, 9:29 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39449/
> ---
> 
> (Updated Oct. 19, 2015, 9:29 p.m.)
> 
> 
> Review request for mesos, Marco Massenzio and Michael Park.
> 
> 
> Bugs: MESOS-2275
> https://issues.apache.org/jira/browse/MESOS-2275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented order of includes.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 0b6189174a4f0f1815625f68fb1a743b04a9cdad 
> 
> Diff: https://reviews.apache.org/r/39449/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

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

(Updated Nov. 3, 2015, 10:30 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Added mention to Credential protobuf change in 0.26 in uprades.md


Diffs (updated)
-

  CHANGELOG 99d8882 
  docs/upgrades.md 3854e59 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 39771: Show the failing path when execvpe(2) fails.

[Ben Mahler]

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



3rdparty/libprocess/src/subprocess.cpp (lines 179 - 180)


Technically we should not be appending strings like this since malloc is 
not async-signal-safe. But since you're not introducing the issue, it's alright.

(1) Typically we would print like this: "Failed to os::execvpe on path 
'': ".

(2) Mind fixing the indent on your second line?


- Ben Mahler


On Oct. 29, 2015, 9 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39771/
> ---
> 
> (Updated Oct. 29, 2015, 9 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3608
> https://issues.apache.org/jira/browse/MESOS-3608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Show the failing path when execvpe(2) fails.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 459825c188d56d25f6e2832e5a170d806e257d6b 
> 
> Diff: https://reviews.apache.org/r/39771/diff/
> 
> 
> Testing
> ---
> 
> make check and manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Vinod Kone]

> On Nov. 3, 2015, 8:10 p.m., Vinod Kone wrote:
> > would this only test libevent and skip libev? if yes, we can't do this.
> 
> Jojy Varghese wrote:
> Would it help to add libev also? I was trying to make this look like the 
> jenkins mesos builds. Console logs there show :
> ``` echo CMD ./bootstrap '&&' ./configure --verbose --enable-libevent 
> --enable-ssl && ...```
> 
> Vinod Kone wrote:
> the jenkins "mesos" build is a parameterized build, where one of the 
> parameters is the configuration options. the config options we test are
> "--enable-libevent --enable-ssl" and "" (empty string which tests libev).
> 
> unfortunately, the review bot doesn't support these parameters yet. 
> review bot doesn't even run inside docker like the "mesos" build. so it just 
> tests one platform (ubuntu) and the default parameters. it would be great if 
> we can fix the review bot to do the same things as "mesos". feel free to 
> create a ticket. i'll be happy to shepherd.
> 
> Jojy Varghese wrote:
> Just created https://issues.apache.org/jira/browse/MESOS-3825. Since 
> there is only 1 platform, do we need to parameterize ? Can we just assume a 
> fixed configuration ? Or add the required packages to the machine/VM and 
> always use --enable-ssl and --enable-libevent ?

thanks for the ticket!

iirc, currently only one of "libev" or "libevent" can be enabled. so, if we 
change the bot to use libevent (because thats a dependency for ssl), then the 
bot won't be testing changes against libev.

regarding installing packages needed for libevent/ssl, we need to file a ticket 
with asf to get those packages installed on all the jenkins hosts, which is a 
PITA. that's the reason why "mesos" build moved to docker and why we need to 
move the bot to docker.

does that make sense?


- Vinod


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


On Nov. 3, 2015, 7:42 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39007: libprocess: Used thread-safe replacement for strerror.

[Benjamin Bannier]

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

(Updated Nov. 3, 2015, 10:53 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, and Till Toenshoff.


Changes
---

Rebased and registered added stout tests.


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


Repository: mesos


Description (updated)
---

Also register test cases for stout's strerror; the Makefile for that is
owned by libprocess.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/Makefile.am 
53e83d4905945593e174601a0b791d01824dc34b 
  3rdparty/libprocess/src/io.cpp 26686e1a96484e3f09d41a7292f38b7579ce9c48 
  3rdparty/libprocess/src/poll_socket.cpp 
85cd864ebddd72316991a8f2782d3c7c9eebcc54 
  3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
  3rdparty/libprocess/src/profiler.cpp 0c515568880aa6b7a65cfe2955eb7132bdfb3baf 
  3rdparty/libprocess/src/subprocess.cpp 
459825c188d56d25f6e2832e5a170d806e257d6b 

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


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 39005: stout: Added thread-safe replacement for strerror.

[Benjamin Bannier]

> On Oct. 8, 2015, 9:07 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp, line 54
> > 
> >
> > We have to support CentOS 5 which has glibc 2.5 FWICT.
> 
> Benjamin Bannier wrote:
> I widdened the requirements on the libc by more granular rejection of the 
> buggy glibc setup. This should work now under e.g. CentOS-5.11.

I am marking this a fixed since I believe this now works under a CentOS5 people 
might still use in production. Please reopen if this still imposes a too strong 
requirement.


- Benjamin


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


On Nov. 3, 2015, 10:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39005/
> ---
> 
> (Updated Nov. 3, 2015, 10:52 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3551
> https://issues.apache.org/jira/browse/MESOS-3551
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit also adds test cases for os::strerror (from stout) which can
> only be committed with a libprocess commit -- the corresponding Makefile
> lives there.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 76e1674e08bbe65a4fdf86731823a61f231d6d12 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> 26e1377ee625748b7fdbec327fef9ac602535f83 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39005/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Vinod Kone]

> On Nov. 3, 2015, 8:06 p.m., Vinod Kone wrote:
> > docs/upgrades.md, line 16
> > 
> >
> > this should also be mentioned in the CHANGELOG.
> 
> Michael Park wrote:
> The CHANGELOG will be created by the release managers for 0.26.0 (Bernd 
> and Till) right? I understand we'll want to make sure this is included in the 
> CHANGELOG for 0.26.0 once it is introduced. Do you think we need to block 
> this patch for it?

We typically update CHANGELOG whenever we make API/incompatible changes. That 
way we don't lose track of this stuff when the release time comes around. While 
Release Managers do a sanity check and dump the JIRA ticket list, it would be 
great if we can help them out whenever we can. This is the same reason we ask 
people to add stuff to "upgrades" asap instead of waiting till the release 
cycle.


- Vinod


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


On Nov. 3, 2015, 7:52 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39914]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 7:42 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39770: Write a newline in ABORT().

[Ben Mahler]

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



3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp (line 53)


Why no EINTR loop on this additional write?


- Ben Mahler


On Oct. 29, 2015, 9:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39770/
> ---
> 
> (Updated Oct. 29, 2015, 9:01 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3708
> https://issues.apache.org/jira/browse/MESOS-3708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Write a newline in ABORT() so that the ABORT message doesn't run
> on into any output from abort(3) and the associated signal backtracing.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
> 2b003d26d6b6b65b1d7b1dd6396f808c35b53177 
> 
> Diff: https://reviews.apache.org/r/39770/diff/
> 
> 
> Testing
> ---
> 
> make check and manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 39922: Add CHANGLOG for 0.26.0 release

[Guangya Liu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This is a WIP CHANGLOG and only including one item of
"Add /frameworks endpoint to master", it should be updated
in future to add more features in 0.26.0.


Diffs
-

  CHANGELOG 99d8882e5093b5e76c983b1f4f84200ac03bb7d5 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Re: Review Request 39005: stout: Added thread-safe replacement for strerror.

[Benjamin Bannier]

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

(Updated Nov. 3, 2015, 10:52 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, and Till Toenshoff.


Changes
---

Rebased and moved os::strerror test cases into separate file.


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


Repository: mesos


Description (updated)
---

This commit also adds test cases for os::strerror (from stout) which can
only be committed with a libprocess commit -- the corresponding Makefile
lives there.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/Makefile.am 
76e1674e08bbe65a4fdf86731823a61f231d6d12 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
67b142bbc2d80f40a1e893cc5813d58dd2aa8381 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/strerror.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
26e1377ee625748b7fdbec327fef9ac602535f83 
  3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField

[Michael Park]

> On Oct. 22, 2015, 8:53 p.m., Jan Schlicht wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 800
> > 
> >
> > Not yours, but please s/push_back/emplace_back
> 
> Michael Park wrote:
> I'm curious as to what your reasoning is?
> 
> Klaus Ma wrote:
> AFAIK, that'll avoid temp var copy from `JSON::protobuf(...)`'s return to 
> `commandArray.values`

Not true. `push_back(T&&)` will be used which avoids the copy. My 
recommendation is to use `push_back` if you have an instance of `T`, and use 
`emplace_back` when you have __constructor parameters__ for `T`. Note that 
using `emplace_back` with an instance of `T` will invoke the copy/move 
constructor so the affect is the same, but they're not interchangeable all 
cases because they have different initialization rules.


- Michael


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


On Nov. 3, 2015, 1:38 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38335/
> ---
> 
> (Updated Nov. 3, 2015, 1:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp d4c05c2 
>   src/examples/persistent_volume_framework.cpp 176ac3d 
>   src/launcher/executor.cpp 50b3c6e 
>   src/master/contender.cpp c641305 
>   src/master/http.cpp 093f793 
>   src/master/maintenance.cpp 5fe9358 
>   src/master/registrar.cpp 1117232 
>   src/slave/containerizer/fetcher.cpp e0d02d5 
>   src/slave/containerizer/mesos/containerizer.cpp 9fd69c1 
>   src/slave/monitor.cpp aa6e958 
>   src/tests/containerizer/launch_tests.cpp de655ec 
>   src/tests/fault_tolerance_tests.cpp f78a291 
>   src/tests/master_maintenance_tests.cpp e89ce3b 
>   src/tests/master_tests.cpp ee24739 
>   src/tests/mesos.cpp ab2d85b 
>   src/tests/monitor_tests.cpp 583e711 
>   src/tests/reservation_endpoints_tests.cpp f5f9c48 
>   src/tests/resources_tests.cpp 6584fc6 
>   src/tests/scheduler_http_api_tests.cpp b6f6e91 
>   src/tests/script.cpp bcc1fab 
>   src/tests/slave_tests.cpp 91dbdba 
>   src/usage/main.cpp 86fd796 
> 
> Diff: https://reviews.apache.org/r/38335/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 39531: Clarify NetworkInfo semantics for IP addresses and group policies.

[Kapil Arya]

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

Ship it!


Ship It!

- Kapil Arya


On Oct. 23, 2015, 3:21 p.m., Connor Doyle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39531/
> ---
> 
> (Updated Oct. 23, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kapil Arya.
> 
> 
> Bugs: MESOS-3788
> https://issues.apache.org/jira/browse/MESOS-3788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In Mesos 0.25.0, a new message called NetworkInfo was introduced.  This 
> message allows framework authors to communicate with network isolation 
> modules via a first-class message type to request IP addresses and network 
> group isolation policies.
> 
> Unfortunately, the structure is somewhat confusing to both framework authors 
> and module implementors.
> 
> 1) It's unclear how IP addresses map to virtual interfaces inside the 
> container.
> 2) It's difficult for application developers to understand the final policy 
> when multiple IP addresses can be assigned with differing isolation policies.
> 
> 
> Diffs
> -
> 
>   docs/networking-for-mesos-managed-containers.md 33568a8 
>   include/mesos/mesos.proto 9400434 
>   include/mesos/v1/mesos.proto 8131778 
>   src/common/http.hpp 0cc98a8 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp 1e49013 
>   src/examples/test_hook_module.cpp 43d6cb9 
>   src/slave/slave.cpp e9f2d1b 
>   src/tests/common/http_tests.cpp c2e7704 
>   src/tests/hook_tests.cpp 5a5d019 
> 
> Diff: https://reviews.apache.org/r/39531/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Connor Doyle
> 
>

Re: Review Request 39531: Clarify NetworkInfo semantics for IP addresses and group policies.

[Kapil Arya]

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



src/common/http.cpp 


Actually, my previous comment wasn't quite clear. We can use 
`JSON::Protobuf` for NetworkInfo::IPAddress, but not for the entire NetworkInfo 
since the labels will show up poorly.


- Kapil Arya


On Oct. 23, 2015, 3:21 p.m., Connor Doyle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39531/
> ---
> 
> (Updated Oct. 23, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kapil Arya.
> 
> 
> Bugs: MESOS-3788
> https://issues.apache.org/jira/browse/MESOS-3788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In Mesos 0.25.0, a new message called NetworkInfo was introduced.  This 
> message allows framework authors to communicate with network isolation 
> modules via a first-class message type to request IP addresses and network 
> group isolation policies.
> 
> Unfortunately, the structure is somewhat confusing to both framework authors 
> and module implementors.
> 
> 1) It's unclear how IP addresses map to virtual interfaces inside the 
> container.
> 2) It's difficult for application developers to understand the final policy 
> when multiple IP addresses can be assigned with differing isolation policies.
> 
> 
> Diffs
> -
> 
>   docs/networking-for-mesos-managed-containers.md 33568a8 
>   include/mesos/mesos.proto 9400434 
>   include/mesos/v1/mesos.proto 8131778 
>   src/common/http.hpp 0cc98a8 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp 1e49013 
>   src/examples/test_hook_module.cpp 43d6cb9 
>   src/slave/slave.cpp e9f2d1b 
>   src/tests/common/http_tests.cpp c2e7704 
>   src/tests/hook_tests.cpp 5a5d019 
> 
> Diff: https://reviews.apache.org/r/39531/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Connor Doyle
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Vinod Kone]

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


would this only test libevent and skip libev? if yes, we can't do this.

- Vinod Kone


On Nov. 3, 2015, 7:42 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Jojy Varghese]

> On Nov. 3, 2015, 8:10 p.m., Vinod Kone wrote:
> > would this only test libevent and skip libev? if yes, we can't do this.

Would it help to add libev also? I was trying to make this look like the 
jenkins mesos builds. Console logs there show :
``` echo CMD ./bootstrap '&&' ./configure --verbose --enable-libevent 
--enable-ssl && ...```


- Jojy


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


On Nov. 3, 2015, 7:42 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39886: Added documentation about roles.

[Neil Conway]

> On Nov. 3, 2015, 8:43 a.m., Alexander Rukletsov wrote:
> > docs/roles.md, line 12
> > 
> >
> > Do you want to be consistent with the existing codebase and continue 
> > use "slave" for now?

We already use "agent" in various places right now (in both code and docs), so 
I think this is okay as-is.


> On Nov. 3, 2015, 8:43 a.m., Alexander Rukletsov wrote:
> > docs/roles.md, line 22
> > 
> >
> > Shall we leave a note here that our intention is to add dynamic roles 
> > ASAP?

Not sure this is that important for user-focused docs. When we add dynamic 
roles, we can update the docs.


> On Nov. 3, 2015, 8:43 a.m., Alexander Rukletsov wrote:
> > docs/roles.md, line 10
> > 
> >
> > Roles also introduce one level in the hierarchy of resource allocation. 
> > Also putting a framework into a role is the only way to leverage some sort 
> > of prioritization (role weights). Similar to what you say at the bottom of 
> > the doc, I would say it makes sense to mention it here as a use case as 
> > well.

Good point! Done.


- Neil


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


On Nov. 3, 2015, 7:51 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 3, 2015, 7:51 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/authorization.md 2a5e74782754e446184a297f91112e9f94077896 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

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

(Updated Nov. 3, 2015, 7:52 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Added mention to Credential protobuf change in 0.26 in uprades.md


Diffs (updated)
-

  docs/upgrades.md 3854e59 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 39886: Added documentation about roles.

[Neil Conway]

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

(Updated Nov. 3, 2015, 7:51 p.m.)


Review request for mesos and Adam B.


Changes
---

Various improvements, per review comments.


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


Repository: mesos


Description
---

Also removed a redundant (and arguably slightly wrong) assertion about the
status of static reservations.


Diffs (updated)
-

  docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
  docs/authorization.md 2a5e74782754e446184a297f91112e9f94077896 
  docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
  docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
  docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
  docs/roles.md PRE-CREATION 

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


Testing
---

Previewed with the support/site Docker container.


Thanks,

Neil Conway

Re: Review Request 39886: Added documentation about roles.

[Neil Conway]

> On Nov. 3, 2015, 2:40 a.m., Guangya Liu wrote:
> > docs/attributes-resources.md, line 49
> > 
> >
> > MESOS-3177 is planning to enable adding role dynamically, we may need 
> > to add a TODO here to reflect this.

My preference is to leave this as-is: if/when dynamic roles are added, we can 
update the docs at that point.


- Neil


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


On Nov. 3, 2015, 7:51 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 3, 2015, 7:51 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/authorization.md 2a5e74782754e446184a297f91112e9f94077896 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Vinod Kone]

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



docs/upgrades.md (line 16)


this should also be mentioned in the CHANGELOG.


- Vinod Kone


On Nov. 3, 2015, 7:52 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39886: Added documentation about roles.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39886]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 7:51 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39886/
> ---
> 
> (Updated Nov. 3, 2015, 7:51 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3819
> https://issues.apache.org/jira/browse/MESOS-3819
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also removed a redundant (and arguably slightly wrong) assertion about the
> status of static reservations.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md f712d094f14426515dabde45f98d6c1ae36c3447 
>   docs/authorization.md 2a5e74782754e446184a297f91112e9f94077896 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
>   docs/home.md af886257348f24cfeecfc726255d8d45d68af2db 
>   docs/reservation.md 69bde760ece59c68b04a903026b5903e2091ceb0 
>   docs/roles.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39886/diff/
> 
> 
> Testing
> ---
> 
> Previewed with the support/site Docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Neil Conway]

> On Nov. 3, 2015, 7:21 p.m., Neil Conway wrote:
> > docs/upgrades.md, line 16
> > 
> >
> > AFAIK this isn't Java-specific.
> 
> Isabel Jimenez wrote:
> This will mostly impact Java frameworks since they'll have to not only 
> update the generated protos but change their framework code. See: 
> 
> https://github.com/apache/mesos/commit/38dbadc944f56e24725fba102bbd2db76cb31228#diff-2ca9e1a2a6f66a790bbbfae40708ecfaL249

Sorry, I'm still confused: updating the generated protos will require changes 
to framework code (if your framework examines the `secret` field), regardless 
of language. e.g., the type of the field has changed from []byte to string in 
the Go generated code.


- Neil


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


On Nov. 3, 2015, 6 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 6 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Joseph Wu]

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


Are openssl and libevent present on the reviewbot machine?  
(And does reviewbot use the modified version of `verify_reviews.py` when 
checking this one?)

- Joseph Wu


On Nov. 3, 2015, 11:42 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 11:42 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

> On Nov. 3, 2015, 7:21 p.m., Neil Conway wrote:
> > docs/upgrades.md, line 16
> > 
> >
> > AFAIK this isn't Java-specific.
> 
> Isabel Jimenez wrote:
> This will mostly impact Java frameworks since they'll have to not only 
> update the generated protos but change their framework code. See: 
> 
> https://github.com/apache/mesos/commit/38dbadc944f56e24725fba102bbd2db76cb31228#diff-2ca9e1a2a6f66a790bbbfae40708ecfaL249
> 
> Neil Conway wrote:
> Sorry, I'm still confused: updating the generated protos will require 
> changes to framework code (if your framework examines the `secret` field), 
> regardless of language. e.g., the type of the field has changed from []byte 
> to string in the Go generated code.

True, let me remove the language specificity.


- Isabel


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


On Nov. 3, 2015, 6 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 6 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Jojy Varghese]

> On Nov. 3, 2015, 7:46 p.m., Joseph Wu wrote:
> > Are openssl and libevent present on the reviewbot machine?  
> > (And does reviewbot use the modified version of `verify_reviews.py` when 
> > checking this one?)

Not sure. I would think that the installation of dependencies exists outside 
the scoep of this file. Maybe dockerfile?


- Jojy


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


On Nov. 3, 2015, 7:42 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39843: Update declineOffer use Call::DECLINE to decline offer

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Nov. 2, 2015, 9:42 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39843/
> ---
> 
> (Updated Nov. 2, 2015, 9:42 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3522
> https://issues.apache.org/jira/browse/MESOS-3522
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update declineOffer use Call::DECLINE to decline offer
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 
>   src/tests/master_tests.cpp ee2473997ccbd1c50d0cbf65d1259ea2dfe82971 
> 
> Diff: https://reviews.apache.org/r/39843/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39634: FreeBSD: Enable mesos build and start fixing some tests

[Alex Clemmer]

> On Nov. 2, 2015, 6:58 p.m., Alex Clemmer wrote:
> > configure.ac, lines 612-621
> > 
> >
> > If it's not too much trouble, it would be great to see this logic added 
> > also the `cmake/CompilationCOnfigure.cmake`. It should only be a couple 
> > lines of code.
> 
> David Forsythe wrote:
> Mind if that happens after/if this change lands? I'm not sure what the 
> ettiquite is for filing tickets on unsupported platforms, but I have no 
> problem bringing the cmake stuff up to date if this gets merged.

Sure, that sounds good, but could I ask you file a JIRA ticket so we can track 
the work? The CMake epic is here: 
https://issues.apache.org/jira/browse/MESOS-898


- Alex


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


On Oct. 30, 2015, 5:05 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39634/
> ---
> 
> (Updated Oct. 30, 2015, 5:05 a.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Bugs: MESOS-1563
> https://issues.apache.org/jira/browse/MESOS-1563
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FreeBSD: Enable mesos build and start fixing some tests
> 
> 
> Diffs
> -
> 
>   configure.ac 656766430e2720c6a407e282521102b54547fd2d 
>   src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db 
>   src/tests/attributes_tests.cpp 4fc0c31c3b2eb745432818c99746a097fde65df3 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/tests/values_tests.cpp e9b1079bbadf05390b39bedd5ad5677f3d4ec0d8 
> 
> Diff: https://reviews.apache.org/r/39634/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 38335: Add JSON::protobuf for google::protobuf::RepeatedPtrField

[Michael Park]

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

Ship it!


Please rebase and address the comment below and I can get this committed for 
you!


src/tests/reservation_endpoints_tests.cpp (lines 107 - 108)


Could we take the `JSON::protobuf(static_cast<...>(resources))` out of this 
expression? It makes it quite difficult to read I think.

Here and below.


- Michael Park


On Nov. 3, 2015, 1:38 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38335/
> ---
> 
> (Updated Nov. 3, 2015, 1:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp d4c05c2 
>   src/examples/persistent_volume_framework.cpp 176ac3d 
>   src/launcher/executor.cpp 50b3c6e 
>   src/master/contender.cpp c641305 
>   src/master/http.cpp 093f793 
>   src/master/maintenance.cpp 5fe9358 
>   src/master/registrar.cpp 1117232 
>   src/slave/containerizer/fetcher.cpp e0d02d5 
>   src/slave/containerizer/mesos/containerizer.cpp 9fd69c1 
>   src/slave/monitor.cpp aa6e958 
>   src/tests/containerizer/launch_tests.cpp de655ec 
>   src/tests/fault_tolerance_tests.cpp f78a291 
>   src/tests/master_maintenance_tests.cpp e89ce3b 
>   src/tests/master_tests.cpp ee24739 
>   src/tests/mesos.cpp ab2d85b 
>   src/tests/monitor_tests.cpp 583e711 
>   src/tests/reservation_endpoints_tests.cpp f5f9c48 
>   src/tests/resources_tests.cpp 6584fc6 
>   src/tests/scheduler_http_api_tests.cpp b6f6e91 
>   src/tests/script.cpp bcc1fab 
>   src/tests/slave_tests.cpp 91dbdba 
>   src/usage/main.cpp 86fd796 
> 
> Diff: https://reviews.apache.org/r/38335/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Michael Park]

> On Nov. 3, 2015, 8:06 p.m., Vinod Kone wrote:
> > docs/upgrades.md, line 16
> > 
> >
> > this should also be mentioned in the CHANGELOG.

The CHANGELOG will be created by the release managers for 0.26.0 (Bernd and 
Till) right? I understand we'll want to make sure this is included in the 
CHANGELOG for 0.26.0 once it is introduced. Do you think we need to block this 
patch for it?


- Michael


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


On Nov. 3, 2015, 7:52 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Jojy Varghese]

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

Review request for mesos, Ben Mahler and Cody Maloney.


Repository: mesos


Description
---

SSL based tests are not currently verified on reviewbot. This change should 
enable those tests to be run.


Diffs
-

  support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 

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


Testing
---


Thanks,

Jojy Varghese

Re: Review Request 39531: Clarify NetworkInfo semantics for IP addresses and group policies.

[Kapil Arya]

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



docs/networking-for-mesos-managed-containers.md (lines 276 - 278)


We'll need to update this comment after https://reviews.apache.org/r/39866/ 
has been merged.


- Kapil Arya


On Oct. 23, 2015, 3:21 p.m., Connor Doyle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39531/
> ---
> 
> (Updated Oct. 23, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kapil Arya.
> 
> 
> Bugs: MESOS-3788
> https://issues.apache.org/jira/browse/MESOS-3788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In Mesos 0.25.0, a new message called NetworkInfo was introduced.  This 
> message allows framework authors to communicate with network isolation 
> modules via a first-class message type to request IP addresses and network 
> group isolation policies.
> 
> Unfortunately, the structure is somewhat confusing to both framework authors 
> and module implementors.
> 
> 1) It's unclear how IP addresses map to virtual interfaces inside the 
> container.
> 2) It's difficult for application developers to understand the final policy 
> when multiple IP addresses can be assigned with differing isolation policies.
> 
> 
> Diffs
> -
> 
>   docs/networking-for-mesos-managed-containers.md 33568a8 
>   include/mesos/mesos.proto 9400434 
>   include/mesos/v1/mesos.proto 8131778 
>   src/common/http.hpp 0cc98a8 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp 1e49013 
>   src/examples/test_hook_module.cpp 43d6cb9 
>   src/slave/slave.cpp e9f2d1b 
>   src/tests/common/http_tests.cpp c2e7704 
>   src/tests/hook_tests.cpp 5a5d019 
> 
> Diff: https://reviews.apache.org/r/39531/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Connor Doyle
> 
>

Re: Review Request 39060: Create master detector per url & not per framework

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39060]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 6:06 p.m., Mandeep Chadha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39060/
> ---
> 
> (Updated Nov. 3, 2015, 6:06 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3595
> https://issues.apache.org/jira/browse/MESOS-3595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the number of framework created exceeds the lib process
> threads then during master failover the zookeeper updates can
> cause deadlock.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp f571d42d1508632152473c4f4ade60ae3900fce1 
>   src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 
> 
> Diff: https://reviews.apache.org/r/39060/diff/
> 
> 
> Testing
> ---
> 
> make check successful 
> Created 100 framework instances on a 24 CPU machine. Master failover detected 
> by the framework process and continue to work as expected.
> 
> 
> Thanks,
> 
> Mandeep Chadha
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Michael Park]

> On Oct. 18, 2015, 12:41 a.m., Michael Park wrote:
> > src/common/resources.cpp, line 377
> > 
> >
> > I think you need to check for `resource.role() == "*"` instead, since 
> > `role` has `[default = "*"]`. Please verify.
> 
> Greg Mann wrote:
> The `has_field` methods return false if the default value was used to 
> initialize the message; I checked this to be sure. This has the added benefit 
> that the method doesn't need to hard-code the protobuf's default role.

Great! Thanks for checking.


- Michael


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


On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Oct. 20, 2015, 6:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

> On Nov. 3, 2015, 11:33 p.m., Guangya Liu wrote:
> > CHANGELOG, line 1
> > 
> >
> > s/Version 0.26.0/Version 0.26.0(WIP)
> 
> Isabel Jimenez wrote:
> This are the release notes, so they apply only for releases. No need to 
> specify WIP
> 
> Guangya Liu wrote:
> The CHANGELOG for 0.26.0 was not finished, I think that we put a `WIP` 
> here and remove it when release 0.26.0, thoughts? Thanks.

My bad, I will specify WIP.


- Isabel


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


On Nov. 3, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十一月 4, 2015, 12:14 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated 十一月 4, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 75f7355 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39007: libprocess: Used thread-safe replacement for strerror.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39005, 39006, 39007]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 10:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39007/
> ---
> 
> (Updated Nov. 3, 2015, 10:53 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3551
> https://issues.apache.org/jira/browse/MESOS-3551
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also register test cases for stout's strerror; the Makefile for that is
> owned by libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 53e83d4905945593e174601a0b791d01824dc34b 
>   3rdparty/libprocess/src/io.cpp 26686e1a96484e3f09d41a7292f38b7579ce9c48 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 85cd864ebddd72316991a8f2782d3c7c9eebcc54 
>   3rdparty/libprocess/src/process.cpp 
> a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
>   3rdparty/libprocess/src/profiler.cpp 
> 0c515568880aa6b7a65cfe2955eb7132bdfb3baf 
>   3rdparty/libprocess/src/subprocess.cpp 
> 459825c188d56d25f6e2832e5a170d806e257d6b 
> 
> Diff: https://reviews.apache.org/r/39007/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39534: SSLTest refactor: Change MesosTest to inherit from the SSL helper class.

[Jie Yu]

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



src/tests/mesos.hpp (line 103)


We have two 'TemporaryDirectoryTest' (one in stout, one in mesos). There's 
some subtle differences between the two (e.g., the temp directory naming, 
umount unneeded mounts)

MesosTest inherits from TemporaryDirectoryTest in Mesos previously. This 
change breaks the test (if you run under root).


- Jie Yu


On Oct. 26, 2015, 10:37 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39534/
> ---
> 
> (Updated Oct. 26, 2015, 10:37 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3762
> https://issues.apache.org/jira/browse/MESOS-3762
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/39534/diff/
> 
> 
> Testing
> ---
> 
> ```
> ../configure
> make check
> ```
> 
> ```
> ../configure --enable-ssl --enable-libevent
> make check
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39908]

All tests passed.

- Mesos ReviewBot


On Nov. 4, 2015, 12:14 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 4, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 75f7355 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Guangya Liu]

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



CHANGELOG (line 1)


s/Version 0.26.0/Version 0.26.0(WIP)



CHANGELOG (line 4)


Why not put this to `Bug` catalog?


- Guangya Liu


On 十一月 3, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated 十一月 3, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

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

(Updated Nov. 4, 2015, 12:14 a.m.)


Review request for mesos and Michael Park.


Changes
---

rebase


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


Repository: mesos


Description
---

Added mention to Credential protobuf change in 0.26 in uprades.md


Diffs (updated)
-

  CHANGELOG 75f7355 
  docs/upgrades.md 3854e59 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.

[Michael Hopcroft]

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


This is my initial set of comments. I still need to read from line 112 on in 
stat.hpp and I haven't started reading reparsepoint.hpp.


3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
(line 14)


Recommend #pragma once. This is supported by VS. GCC supports it since 
version 3.4 (https://en.wikipedia.org/wiki/Pragma_once), but this shouldn't 
matter since we're discussing a Windows file. In general #pragma once is 
preferred over the #ifndef include guard mechanism, so the main question would 
pertain to consistency with the rest of the codebase. My recommendation is to 
use #pragma once for all new code and gradually migrate old code as it is 
edited.

I notice that the Google Style Guide recommends the #ifndef version. This 
may be a factor in deciding.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
(line 22)


Not sure how lines 22 and 24 fit with the #include file ordering scheme.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
(line 55)


Google Style Guide allows auto if it help readability? Is it a good or bad 
idea here?



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
(line 61)


Recommend opening a work item for Windows team to add this API. Who knows 
what version of Windows would actually ship it, but the world would be a 
slightly better place.

Also wonder if you should add your code to a StackOverflow answer.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
(line 64)


Google Style Guide isn't really clear on this one. I would recommend 
indenting line 66 to align with the open paren on line 65.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
(line 69)


I'd put a shorter comment here, something like "Open `HANDLE` for the 
symlink isself." In general I feel it is error prone to include a verbatem copy 
of the function's documentation here as it won't get updated, but could be 
confused for the function's documentation.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
(line 81)


Is it possible to leak a handle in the presence of an error? I guess if 
getSymbolicLinkData() can never throw you will always execute line 81.

Still, I would rather see RAII pattern involving any code with handles, 
just to be safe. This also future proofs the code against edits that make the 
logic more complex.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 17)


A quick read doesn't turn up any use of . Since I don't really 
know the stout API all that well, I would recommend attempting a compile with 
each new header removed to generate proof that they are being used. Also keep 
in mind that they must be required by stat.hpp, and not some other header than 
includes it.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 39)


Might add a comment explaining that when ::stat() returns a value less than 
zero the error can only be ENOENT or EINVAL. ENOENT means the path doesn't 
exist, so returning false is correct. EINVAL should be impossible, based on 
inspection of the code.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 62)


If you always return symlink.isSome(), then why use Try<>? Is it that other 
users of querySymbolicLinkData() actually care about getting an error back?

Even if you keep Try<> in the API, is it ok to silently ignore an error?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 68)


Is FollowSymLink your enum or an enum that already exists in the Linux 
version?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 75)


When you say the size of the file system entry, I assume you mean the size 
of the file pointed to, not the number of bytes in the file system record. 
Might want to reword the comment to make it more clear.

Also, what happens if the path is a directory?

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

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

(Updated Nov. 3, 2015, 11:41 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Added mention to Credential protobuf change in 0.26 in uprades.md


Diffs (updated)
-

  CHANGELOG 99d8882 
  docs/upgrades.md 3854e59 

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


Testing
---


Thanks,

Isabel Jimenez

Review Request 39923: Cleaned up configuration.md.

[Greg Mann]

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

Review request for mesos, Jojy Varghese, Joris Van Remoortere, and Kapil Arya.


Repository: mesos


Description
---

Edited configuration.md for style and consistency. Made consistent use of 
 for commands, flags, and flag values. Edited the libprocess flags 
for consistency and proper use of `=DIR` or `[=DIR]` depending on whether or 
not the directory is an optional parameter. Note that the libprocess `--with-*` 
flags have two different descriptions depending upon whether or not the library 
is bundled with Mesos.


Diffs
-

  docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 

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


Testing
---

Viewed with the mesos website container 
(https://github.com/mesosphere/mesos-website-container).


Thanks,

Greg Mann

Re: Review Request 39770: Write a newline in ABORT().

[James Peach]

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

(Updated Nov. 4, 2015, 12:25 a.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Write a newline in ABORT() so that the ABORT message doesn't run
on into any output from abort(3) and the associated signal backtracing.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
2b003d26d6b6b65b1d7b1dd6396f808c35b53177 

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


Testing
---

make check and manual testing.


Thanks,

James Peach

Re: Review Request 39771: Show the failing path when execvpe(2) fails.

[James Peach]

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

(Updated Nov. 4, 2015, 12:24 a.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Show the failing path when execvpe(2) fails.


Diffs (updated)
-

  3rdparty/libprocess/src/subprocess.cpp 
459825c188d56d25f6e2832e5a170d806e257d6b 

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


Testing
---

make check and manual testing.


Thanks,

James Peach

Re: Review Request 39923: Cleaned up configuration.md.

[Greg Mann]

> On Nov. 4, 2015, 12:29 a.m., Guangya Liu wrote:
> > docs/configuration.md, line 792
> > 
> >
> > Why remove this?

The text already states "A path to a file containing either one of the above 
options.", and I thought that it makes this line a bit redundant.


- Greg


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


On Nov. 4, 2015, 12:16 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39923/
> ---
> 
> (Updated Nov. 4, 2015, 12:16 a.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Joris Van Remoortere, and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Edited configuration.md for style and consistency. Made consistent use of 
>  for commands, flags, and flag values. Edited the libprocess 
> flags for consistency and proper use of `=DIR` or `[=DIR]` depending on 
> whether or not the directory is an optional parameter. Note that the 
> libprocess `--with-*` flags have two different descriptions depending upon 
> whether or not the library is bundled with Mesos.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
> 
> Diff: https://reviews.apache.org/r/39923/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the mesos website container 
> (https://github.com/mesosphere/mesos-website-container).
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39908]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39922: Update CHANGELOG for 0.26.0 release

[Vinod Kone]

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

Ship it!



CHANGELOG (line 5)


s/Bug/API Changes/

i'll fix this for you when committing.


- Vinod Kone


On Nov. 3, 2015, 11:18 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39922/
> ---
> 
> (Updated Nov. 3, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3824
> https://issues.apache.org/jira/browse/MESOS-3824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a WIP CHANGELOG and only including one item of
> "Add /frameworks endpoint to master", it should be updated
> in future to add more features in 0.26.0.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882e5093b5e76c983b1f4f84200ac03bb7d5 
> 
> Diff: https://reviews.apache.org/r/39922/diff/
> 
> 
> Testing
> ---
> 
> Ubuntu 14.04
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Guangya Liu]

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



docs/upgrades.md (line 16)


I think you should identify that only JSON format is affected?


- Guangya Liu


On 十一月 3, 2015, 11:41 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated 十一月 3, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39841: WIP: Used cgroups::enabled() to check for the availability of the freezer.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Nov. 2, 2015, 1:36 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39841/
> ---
> 
> (Updated Nov. 2, 2015, 1:36 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3814
> https://issues.apache.org/jira/browse/MESOS-3814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Used cgroups::enabled() to check for the availability of the freezer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/linux_launcher.cpp 
> c0adb34771fdb5a85d087296a8f98b890254ddf7 
> 
> Diff: https://reviews.apache.org/r/39841/diff/
> 
> 
> Testing
> ---
> 
> This should solve the problem reported in 
> https://issues.apache.org/jira/browse/MESOS-3814, however with this test in 
> Docker still fail. The reason is that `freezer` subsytem is actually enabled 
> in docker, however the Linux launcher fails because the file system is not 
> mounted.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 39922: Update CHANGELOG for 0.26.0 release

[Guangya Liu]

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

(Updated 十一月 3, 2015, 11:18 p.m.)


Review request for mesos and Vinod Kone.


Summary (updated)
-

Update CHANGELOG for 0.26.0 release


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


Repository: mesos


Description (updated)
---

This is a WIP CHANGELOG and only including one item of
"Add /frameworks endpoint to master", it should be updated
in future to add more features in 0.26.0.


Diffs
-

  CHANGELOG 99d8882e5093b5e76c983b1f4f84200ac03bb7d5 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

> On Nov. 3, 2015, 11:33 p.m., Guangya Liu wrote:
> > CHANGELOG, line 1
> > 
> >
> > s/Version 0.26.0/Version 0.26.0(WIP)

This are the release notes, so they apply only for releases. No need to specify 
WIP


- Isabel


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


On Nov. 3, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Guangya Liu]

> On 十一月 3, 2015, 11:33 p.m., Guangya Liu wrote:
> > CHANGELOG, line 1
> > 
> >
> > s/Version 0.26.0/Version 0.26.0(WIP)
> 
> Isabel Jimenez wrote:
> This are the release notes, so they apply only for releases. No need to 
> specify WIP

The CHANGELOG for 0.26.0 was not finished, I think that we put a `WIP` here and 
remove it when release 0.26.0, thoughts? Thanks.


- Guangya


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


On 十一月 3, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated 十一月 3, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

> On Nov. 3, 2015, 11:33 p.m., Guangya Liu wrote:
> > CHANGELOG, line 4
> > 
> >
> > Why not put this to `Bug` catalog?

As this is an API breaking change we prefered to refer to it under 'API 
changes' even if the JIRA type is orgininally a bug


- Isabel


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


On Nov. 3, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated Nov. 3, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39908: Added a section to Upgrades doc

[Isabel Jimenez]

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

(Updated Nov. 3, 2015, 11:51 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Added mention to Credential protobuf change in 0.26 in uprades.md


Diffs (updated)
-

  CHANGELOG 99d8882 
  docs/upgrades.md 3854e59 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 39922: Updated CHANGELOG for 0.26.0 release.

[Guangya Liu]

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

(Updated 十一月 3, 2015, 11:51 p.m.)


Review request for mesos and Vinod Kone.


Summary (updated)
-

Updated CHANGELOG for 0.26.0 release.


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


Repository: mesos


Description
---

This is a WIP CHANGELOG and only including one item of
"Add /frameworks endpoint to master", it should be updated
in future to add more features in 0.26.0.


Diffs
-

  CHANGELOG 99d8882e5093b5e76c983b1f4f84200ac03bb7d5 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Re: Review Request 39908: Added a section to Upgrades doc

[Guangya Liu]

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



CHANGELOG (lines 1 - 8)


Please rebase based on https://reviews.apache.org/r/39922/


- Guangya Liu


On 十一月 3, 2015, 11:51 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39908/
> ---
> 
> (Updated 十一月 3, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3560
> https://issues.apache.org/jira/browse/MESOS-3560
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mention to Credential protobuf change in 0.26 in uprades.md
> 
> 
> Diffs
> -
> 
>   CHANGELOG 99d8882 
>   docs/upgrades.md 3854e59 
> 
> Diff: https://reviews.apache.org/r/39908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 39771: Show the failing path when execvpe(2) fails.

[James Peach]

> On Nov. 3, 2015, 10:45 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 179-180
> > 
> >
> > Technically we should not be appending strings like this since malloc 
> > is not async-signal-safe. But since you're not introducing the issue, it's 
> > alright.
> > 
> > (1) Typically we would print like this: "Failed to os::execvpe on path 
> > '': ".
> > 
> > (2) Mind fixing the indent on your second line?

I could make _Abort() take a nul-terminated varargs list of string constants. 
That would get rid of the allocation :)


- James


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


On Oct. 29, 2015, 9 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39771/
> ---
> 
> (Updated Oct. 29, 2015, 9 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3608
> https://issues.apache.org/jira/browse/MESOS-3608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Show the failing path when execvpe(2) fails.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 459825c188d56d25f6e2832e5a170d806e257d6b 
> 
> Diff: https://reviews.apache.org/r/39771/diff/
> 
> 
> Testing
> ---
> 
> make check and manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39771: Show the failing path when execvpe(2) fails.

[James Peach]

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

(Updated Nov. 4, 2015, 12:21 a.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Show the failing path when execvpe(2) fails.


Diffs
-

  3rdparty/libprocess/src/subprocess.cpp 
459825c188d56d25f6e2832e5a170d806e257d6b 

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


Testing
---

make check and manual testing.


Thanks,

James Peach

Re: Review Request 39923: Cleaned up configuration.md.

[Guangya Liu]

> On 十一月 4, 2015, 12:29 a.m., Guangya Liu wrote:
> > docs/configuration.md, line 792
> > 
> >
> > Why remove this?
> 
> Greg Mann wrote:
> The text already states "A path to a file containing either one of the 
> above options.", and I thought that it makes this line a bit redundant.

Fair enogh. Thanks.


- Guangya


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


On 十一月 4, 2015, 12:16 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39923/
> ---
> 
> (Updated 十一月 4, 2015, 12:16 a.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Joris Van Remoortere, and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Edited configuration.md for style and consistency. Made consistent use of 
>  for commands, flags, and flag values. Edited the libprocess 
> flags for consistency and proper use of `=DIR` or `[=DIR]` depending on 
> whether or not the directory is an optional parameter. Note that the 
> libprocess `--with-*` flags have two different descriptions depending upon 
> whether or not the library is bundled with Mesos.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
> 
> Diff: https://reviews.apache.org/r/39923/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the mesos website container 
> (https://github.com/mesosphere/mesos-website-container).
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Guangya Liu]

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



src/v1/resources.cpp (line 1144)


remove this


- Guangya Liu


On 十一月 3, 2015, 10:55 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated 十一月 3, 2015, 10:55 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39799: Allow hdfs URLs in the HDFS wrapper API.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Oct. 30, 2015, 2:59 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39799/
> ---
> 
> (Updated Oct. 30, 2015, 2:59 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3602
> https://issues.apache.org/jira/browse/MESOS-3602
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow hdfs URLs in the HDFS wrapper API.
> 
> 
> Diffs
> -
> 
>   src/hdfs/hdfs.hpp 18f17231b92b84d0b0e4e15837d0e44ce8758cdf 
> 
> Diff: https://reviews.apache.org/r/39799/diff/
> 
> 
> Testing
> ---
> 
> make check. Manual testing with a framework that uses the fetcher with HDFS 
> urls.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39771: Show the failing path when execvpe(2) fails.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39770, 39771]

All tests passed.

- Mesos ReviewBot


On Nov. 4, 2015, 12:26 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39771/
> ---
> 
> (Updated Nov. 4, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3708
> https://issues.apache.org/jira/browse/MESOS-3708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Show the failing path when execvpe(2) fails.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 459825c188d56d25f6e2832e5a170d806e257d6b 
> 
> Diff: https://reviews.apache.org/r/39771/diff/
> 
> 
> Testing
> ---
> 
> make check and manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39800: Fix HDFS du output parsing.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Oct. 30, 2015, 3:12 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39800/
> ---
> 
> (Updated Oct. 30, 2015, 3:12 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3605
> https://issues.apache.org/jira/browse/MESOS-3605
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Swallow WARN log messages unless the command failed or we could
>not parse the output.
>  - Make the actual du output parsing more reliable by not using
>human-readable format, scanning all the output lines, and
>tokenizing rather than splitting the output lines.
> 
> 
> Diffs
> -
> 
>   src/hdfs/hdfs.hpp 18f17231b92b84d0b0e4e15837d0e44ce8758cdf 
>   src/tests/fetcher_tests.cpp 46d0690887b6b6af4c7102753cae8db0a375fa08 
> 
> Diff: https://reviews.apache.org/r/39800/diff/
> 
> 
> Testing
> ---
> 
> make check. Manual testing with a framework that uses the fetcher with HDFS 
> urls.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39923: Cleaned up configuration.md.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39923]

All tests passed.

- Mesos ReviewBot


On Nov. 4, 2015, 12:16 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39923/
> ---
> 
> (Updated Nov. 4, 2015, 12:16 a.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Joris Van Remoortere, and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Edited configuration.md for style and consistency. Made consistent use of 
>  for commands, flags, and flag values. Edited the libprocess 
> flags for consistency and proper use of `=DIR` or `[=DIR]` depending on 
> whether or not the directory is an optional parameter. Note that the 
> libprocess `--with-*` flags have two different descriptions depending upon 
> whether or not the library is bundled with Mesos.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
> 
> Diff: https://reviews.apache.org/r/39923/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the mesos website container 
> (https://github.com/mesosphere/mesos-website-container).
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39878: Updated code and comments Master::updateTask() that deals with 0.21.0 slaves.

[Ben Mahler]

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

Ship it!



src/master/master.cpp (line 6025)


Can we pull this comment and the one at 6031 up into a higher level comment 
above the Option? Instead of what we currently say on top of the Option which 
is pretty redundant with what the code expresses.

```
  // Slave-generated updates will contain a 'latest_state'. 
  Option latestState;
  if (update.has_latest_state()) {
latestState = update.latest_state();
  }

  bool terminated;
  
  if (latestState.isSome()) {
...
  } else {
...
  }
```


- Ben Mahler


On Nov. 2, 2015, 9:49 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39878/
> ---
> 
> (Updated Nov. 2, 2015, 9:49 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The only functional change here is that updateTask() doesn't check for uuid 
> being set to empty string.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea 
> 
> Diff: https://reviews.apache.org/r/39878/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 39873: Fixed master to properly handle status updates when multiple of them are enqueued on the slave.

[Ben Mahler]

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



src/master/master.cpp (lines 6004 - 6008)


Just looking at the patch that introduced this bug, why are we removing the 
out-of-order update prevention? Don't see any mention of this in the stuff 
around https://issues.apache.org/jira/browse/MESOS-2864.



src/master/master.cpp (line 6034)


Why not have the same logic here to be defensive? Or do you intend to guard 
against it? Just seems weird to allow it in one of the cases but not the other.


- Ben Mahler


On Nov. 2, 2015, 10:34 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39873/
> ---
> 
> (Updated Nov. 2, 2015, 10:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2864
> https://issues.apache.org/jira/browse/MESOS-2864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master now doesn't change the latest state of a task if it has already 
> terminated. But it still updates the status update state and uuid.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2bc5a97a5b50c8a8a9902c47b2e9e3b5216d97ea 
> 
> Diff: https://reviews.apache.org/r/39873/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran the "RecoverCompletedExecutor" test 100 times as it was failing most of 
> the time without this fix.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Michael Park]

> On Oct. 18, 2015, 6:18 p.m., Michael Park wrote:
> > src/common/resources.cpp, lines 482-532
> > 
> >
> > How about a structure like the following?
> > 
> > ```cpp
> > Resources result;
> > 
> > // Populate `result` based on which format.
> > Try resourcesJSON = JSON::parse(text);
> > if (resourcesJSON.isSome()) {
> >   result = internal::convertJSON(resourcesJSON.get(), defaultRole);
> > } else {
> >   foreach (...) {
> > ...
> > result += resource.get();
> >   }
> > }
> > 
> > // Validate the result regardless of what format
> > Option validate = validateCommandLineResources(result);
> > if (validate.isSome()) {
> >   return validate.get();
> > }
> > 
> > return result;
> > ```
> > 
> > `validateCommandLineResources` is as suggested before, with the 
> > `conflictingTypes` logic encapsulated within it.
> > 
> > I noticed we only perform the minimal validation necessary for the 
> > semicolon-delimited string format currently.
> > That is, we only do the `conflictingTypes` check. While this is 
> > sufficient in the current state of the format,
> > I think it simplifies the code to just do the full check. It's also 
> > future-proof if we need to extend the string format later on.
> 
> Greg Mann wrote:
> Yep I agree that this is cleaner. I implemented it and unfortunately, 
> since `internal::convertJSON` returns a `Try`, we have to unwrap 
> the Try before assigning it to `result`. We could get around this by wrapping 
> the `if ... else` block in a lambda and assigning the result to a 
> `Try result`. In that case, however, we end up wrapping the 
> old-style parsed resources in a `Try` unnecessarily, so we do extra work 
> either way. I think the current version (without the lambda) is a bit more 
> readable, so I went with that one.

This is totally fine. It's no worse than what we do in the old-style resources 
where we unwrap each of the `Try` to build the `result`:
```cpp
Try resource = Resources::parse(name, pair[1], role);
if (resource.isError()) {
  return Error(resource.error());
}

result += resource.get();
```
Pragramatically speaking, we also only do this on start-up for command-line 
parsing and maybe in tests. Not a big deal if we're not perf-optimized here.


- Michael


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


On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Oct. 20, 2015, 6:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Vinod Kone]

> On Nov. 3, 2015, 8:10 p.m., Vinod Kone wrote:
> > would this only test libevent and skip libev? if yes, we can't do this.
> 
> Jojy Varghese wrote:
> Would it help to add libev also? I was trying to make this look like the 
> jenkins mesos builds. Console logs there show :
> ``` echo CMD ./bootstrap '&&' ./configure --verbose --enable-libevent 
> --enable-ssl && ...```

the jenkins "mesos" build is a parameterized build, where one of the parameters 
is the configuration options. the config options we test are
"--enable-libevent --enable-ssl" and "" (empty string which tests libev).

unfortunately, the review bot doesn't support these parameters yet. review bot 
doesn't even run inside docker like the "mesos" build. so it just tests one 
platform (ubuntu) and the default parameters. it would be great if we can fix 
the review bot to do the same things as "mesos". feel free to create a ticket. 
i'll be happy to shepherd.


- Vinod


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


On Nov. 3, 2015, 7:42 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39914: Added enable-ssl and enable-libevent to reviewbot builds

[Jojy Varghese]

> On Nov. 3, 2015, 8:10 p.m., Vinod Kone wrote:
> > would this only test libevent and skip libev? if yes, we can't do this.
> 
> Jojy Varghese wrote:
> Would it help to add libev also? I was trying to make this look like the 
> jenkins mesos builds. Console logs there show :
> ``` echo CMD ./bootstrap '&&' ./configure --verbose --enable-libevent 
> --enable-ssl && ...```
> 
> Vinod Kone wrote:
> the jenkins "mesos" build is a parameterized build, where one of the 
> parameters is the configuration options. the config options we test are
> "--enable-libevent --enable-ssl" and "" (empty string which tests libev).
> 
> unfortunately, the review bot doesn't support these parameters yet. 
> review bot doesn't even run inside docker like the "mesos" build. so it just 
> tests one platform (ubuntu) and the default parameters. it would be great if 
> we can fix the review bot to do the same things as "mesos". feel free to 
> create a ticket. i'll be happy to shepherd.

Just created https://issues.apache.org/jira/browse/MESOS-3825. Since there is 
only 1 platform, do we need to parameterize ? Can we just assume a fixed 
configuration ? Or add the required packages to the machine/VM and always use 
--enable-ssl and --enable-libevent ?


- Jojy


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


On Nov. 3, 2015, 7:42 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39914/
> ---
> 
> (Updated Nov. 3, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Cody Maloney.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SSL based tests are not currently verified on reviewbot. This change should 
> enable those tests to be run.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py 0d322a19af719aba8846fbc705bf8ec7647304a3 
> 
> Diff: https://reviews.apache.org/r/39914/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>