Re: Review Request 46923: Added framework failover timeout validation.

2016-05-12 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [46923]

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

- Mesos ReviewBot


On May 11, 2016, 3:47 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 11, 2016, 3:47 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 35b428b0f7dee5954514d8860cfc498271ccf267 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-12 Thread Vinod Kone 

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


Ship it!




Ship It!

- Vinod Kone


On May 11, 2016, 3:47 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 11, 2016, 3:47 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 35b428b0f7dee5954514d8860cfc498271ccf267 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-10 Thread Jose Guilherme Vanz 

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

(Updated May 11, 2016, 3:47 a.m.)


Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
Kone.


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


Repository: mesos


Description
---

Frameworks were allowed to subscribe themselves with invalid failover
timeout. For this reason, a validation has been made in the master to
deny framework subscription if it set a invalid value for the failover
timeout. MESOS-1575


Diffs (updated)
-

  src/master/master.cpp 35b428b0f7dee5954514d8860cfc498271ccf267 
  src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-10 Thread Vinod Kone 

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




src/master/master.hpp (line 929)


why does this need to be a member function? can it just be a static helper 
function inside master.cpp?



src/master/master.cpp (line 1386)


s/we/We/

All comments should start with a capital letter and end with a period.



src/master/master.cpp (lines 7334 - 7336)


can this just be

```
return Duration::Create(frameworknfo.failover_timeout()).isSome()?
```

at this point, do we even need to extract this out into a function at all?



src/tests/master_tests.cpp (line 4473)


2 blank lines between outer elements.


- Vinod Kone


On May 6, 2016, 12:09 p.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 6, 2016, 12:09 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-06 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [46923]

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

- Mesos ReviewBot


On May 6, 2016, 12:09 p.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 6, 2016, 12:09 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-06 Thread Jose Guilherme Vanz 

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

(Updated May 6, 2016, 12:09 p.m.)


Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
Kone.


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


Repository: mesos


Description
---

Frameworks were allowed to subscribe themselves with invalid failover
timeout. For this reason, a validation has been made in the master to
deny framework subscription if it set a invalid value for the failover
timeout. MESOS-1575


Diffs (updated)
-

  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
  src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-06 Thread Neil Conway 

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




src/master/master.cpp (line 1387)


Comments should be terminated with a period.



src/tests/master_tests.cpp (line 4484)


Comments should be terminated with a period.


- Neil Conway


On May 6, 2016, 12:03 p.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 6, 2016, 12:03 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-06 Thread Jose Guilherme Vanz 

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

(Updated May 6, 2016, 12:03 p.m.)


Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
Kone.


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


Repository: mesos


Description
---

Frameworks were allowed to subscribe themselves with invalid failover
timeout. For this reason, a validation has been made in the master to
deny framework subscription if it set a invalid value for the failover
timeout. MESOS-1575


Diffs (updated)
-

  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
  src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-06 Thread Neil Conway 

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


Fix it, then Ship it!





src/master/master.cpp (line 1386)


I think we should add a comment that says we can safely assume that the 
`failover_timeout` is valid because we registration with an invalid 
`failover_timeout` is refused.


Otherwise, LGTM!

- Neil Conway


On May 6, 2016, 3:18 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 6, 2016, 3:18 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-05 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [46923]

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

- Mesos ReviewBot


On May 6, 2016, 3:18 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 6, 2016, 3:18 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-05 Thread Jose Guilherme Vanz 

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

(Updated May 6, 2016, 3:18 a.m.)


Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
Kone.


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


Repository: mesos


Description
---

Frameworks were allowed to subscribe themselves with invalid failover
timeout. For this reason, a validation has been made in the master to
deny framework subscription if it set a invalid value for the failover
timeout. MESOS-1575


Diffs (updated)
-

  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
  src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-05 Thread Jose Guilherme Vanz 


> On May 4, 2016, 7:35 a.m., Neil Conway wrote:
> > I notice that `Master::_exited` also checks that the `failover_timeout` is 
> > valid. Do you think it would be better to simplify that logic to assume 
> > that the `failover_timeout` is always valid? i.e., add a `CHECK` that it is 
> > valid rather than printing a warning message.

Yeah, It's a good ideia. I'll upload a new diff soon and you can review again.


> On May 4, 2016, 7:35 a.m., Neil Conway wrote:
> > src/tests/master_tests.cpp, line 4481
> > 
> >
> > This comment still seems unclear to me -- we should explain why 
> > `9` is an invalid framework timeout. (The reason is that we 
> > internally represent the timeout using nanoseconds since the epoch as an 
> > int64; `9` in seconds converted to nanoseconds is too large 
> > for an int64). Also, the conversion fails internally to Mesos (stout), not 
> > because of protobuf.

I got it. Sorry for  misunderstanding. I'll also take a look in stout more 
carefully soon


- Jose Guilherme


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


On May 4, 2016, 12:43 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 4, 2016, 12:43 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-04 Thread Neil Conway 

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



I notice that `Master::_exited` also checks that the `failover_timeout` is 
valid. Do you think it would be better to simplify that logic to assume that 
the `failover_timeout` is always valid? i.e., add a `CHECK` that it is valid 
rather than printing a warning message.


src/master/master.hpp (line 938)


You can remove this change.



src/master/master.cpp (line 2256)


I think using `stringify` here would be a bit more idiomatic in Mesos.



src/master/master.cpp (line 2479)


This should be consistent with the first instance of the error message 
above.



src/tests/master_tests.cpp (line 4481)


This comment still seems unclear to me -- we should explain why 
`9` is an invalid framework timeout. (The reason is that we 
internally represent the timeout using nanoseconds since the epoch as an int64; 
`9` in seconds converted to nanoseconds is too large for an 
int64). Also, the conversion fails internally to Mesos (stout), not because of 
protobuf.


- Neil Conway


On May 4, 2016, 12:43 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 4, 2016, 12:43 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-03 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [46923]

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

- Mesos ReviewBot


On May 4, 2016, 12:43 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 4, 2016, 12:43 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Kevin Sweeney, Neil Conway, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1575
> https://issues.apache.org/jira/browse/MESOS-1575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-03 Thread Jose Guilherme Vanz 

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

(Updated May 4, 2016, 12:31 a.m.)


Review request for mesos, Kevin Sweeney and Vinod Kone.


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


Repository: mesos


Description
---

Frameworks were allowed to subscribe themselves with invalid failover
timeout. For this reason, a validation has been made in the master to
deny framework subscription if it set a invalid value for the failover
timeout. MESOS-1575


Diffs (updated)
-

  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
  src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-03 Thread Jose Guilherme Vanz 

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

(Updated May 4, 2016, 12:27 a.m.)


Review request for mesos, Kevin Sweeney and Vinod Kone.


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


Repository: mesos


Description
---

Frameworks were allowed to subscribe themselves with invalid failover
timeout. For this reason, a validation has been made in the master to
deny framework subscription if it set a invalid value for the failover
timeout. MESOS-1575


Diffs
-

  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
  src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-03 Thread Guangya Liu 

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



You can put `MESOS-1575` to the `bugs` column at right.


src/master/master.cpp (line 7341)


s/failoverTimeout_/failoverTimeout



src/master/master.cpp (line 7342)


two spaces is enough


- Guangya Liu


On 五月 3, 2016, 3:31 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated 五月 3, 2016, 3:31 a.m.)
> 
> 
> Review request for mesos, Kevin Sweeney and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-03 Thread Neil Conway 

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




src/master/master.hpp (line 937)


The preceding comment ("Inner class used to namespace the handling of quota 
requests...") refers to the `QuotaHandler` code below -- you should move this 
function up before the comment.



src/master/master.cpp (line 2255)


Indented too far to the right (two spaces per indent level is Mesos style).

I'd use "failover_timeout", rather than "failover timeout".

Should we include the framework-provided timeout value in the error 
message? I'd think probably.



src/master/master.cpp (line 2476)


We probably want to check `validationError.isNone()` here.



src/master/master.cpp (line 7342)


Whitespace.



src/tests/master_tests.cpp (line 4481)


Can we add a comment describing why this failover_timeout is invalid?


- Neil Conway


On May 3, 2016, 3:31 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 3, 2016, 3:31 a.m.)
> 
> 
> Review request for mesos, Kevin Sweeney and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 46923: Added framework failover timeout validation.

2016-05-03 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [46923]

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

- Mesos ReviewBot


On May 3, 2016, 3:31 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46923/
> ---
> 
> (Updated May 3, 2016, 3:31 a.m.)
> 
> 
> Review request for mesos, Kevin Sweeney and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Frameworks were allowed to subscribe themselves with invalid failover
> timeout. For this reason, a validation has been made in the master to
> deny framework subscription if it set a invalid value for the failover
> timeout. MESOS-1575
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
>   src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 
> 
> Diff: https://reviews.apache.org/r/46923/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Review Request 46923: Added framework failover timeout validation.

2016-05-02 Thread Jose Guilherme Vanz 

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

Review request for mesos.


Repository: mesos


Description
---

Frameworks were allowed to subscribe themselves with invalid failover
timeout. For this reason, a validation has been made in the master to
deny framework subscription if it set a invalid value for the failover
timeout. MESOS-1575


Diffs
-

  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
  src/tests/master_tests.cpp 8e00753fcbcd0cae1d08aad387b08aa17c7f63ad 

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


Testing
---


Thanks,

Jose Guilherme Vanz