Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 24, 2016, 4:14 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
8d0070a1b8e8dcc7fe6360f8c6c6182ba9edef7d 

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


Testing
---

On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="TaskValidationTest.ExecutorShutdownGracePeriodIsNonNegative" 
./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Alexander Rukletsov]

> On March 23, 2016, 6:49 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 1175-1176
> > 
> >
> > Why the explicit detector?

After recent test harness refactoring, there is no `StartSlave()` overload 
creating detector implicitly.


- Alexander


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


On March 22, 2016, 5:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 22, 2016, 5:05 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> 8d0070a1b8e8dcc7fe6360f8c6c6182ba9edef7d 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> `make check`
> `GTEST_FILTER="TaskValidationTest.ExecutorShutdownGracePeriodIsNonNegative" 
> ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Ben Mahler]

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


Fix it, then Ship it!




Thanks for moving the test!


src/tests/master_validation_tests.cpp (lines 1175 - 1176)


Why the explicit detector?


- Ben Mahler


On March 22, 2016, 5:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 22, 2016, 5:05 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> 8d0070a1b8e8dcc7fe6360f8c6c6182ba9edef7d 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> `make check`
> `GTEST_FILTER="TaskValidationTest.ExecutorShutdownGracePeriodIsNonNegative" 
> ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 22, 2016, 10:05 a.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
8d0070a1b8e8dcc7fe6360f8c6c6182ba9edef7d 

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


Testing (updated)
---

On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="TaskValidationTest.ExecutorShutdownGracePeriodIsNonNegative" 
./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 21, 2016, 6:38 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Added a test.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
8d0070a1b8e8dcc7fe6360f8c6c6182ba9edef7d 

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


Testing
---

On Mac OS 10.10.1:
`make check`
`GTEST_FILTER="TaskValidationTest.ExecutorShutdownGracePeriodIsNonNegative" 
./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 21, 2016, 2:28 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 

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


Testing (updated)
---

On Mac OS 10.10.1:
`make check`
`GTEST_FILTER="TaskValidationTest.ExecutorShutdownGracePeriodIsNonNegative" 
./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 17, 2016, 11:34 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Ben Mahler]

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


Ship it!




The change looks good but please add a test for this.

Ideally there could be a stateless validateExecutorInfo, in addition to the one 
that needs the master state. In the absence of this, you can write a test that 
spins up a framework and launches a task with a negative shutdown duration.

- Ben Mahler


On March 17, 2016, 11:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 17, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Ben Mahler]

> On March 18, 2016, 9:33 p.m., Ben Mahler wrote:
> > The change looks good but please add a test for this.
> > 
> > Ideally there could be a stateless validateExecutorInfo, in addition to the 
> > one that needs the master state. In the absence of this, you can write a 
> > test that spins up a framework and launches a task with a negative shutdown 
> > duration.

I see now that you tested this in https://reviews.apache.org/r/44994/ inside a 
larger test, would have been helpful to mention that here since the testing 
done section points to https://reviews.apache.org/r/44662 which seems to be 
only a CHANGELOG change..?


- Ben


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


On March 17, 2016, 11:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 17, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44854: Added validation for executor's shutdown grace period.

[Ben Mahler]

> On March 18, 2016, 9:33 p.m., Ben Mahler wrote:
> > The change looks good but please add a test for this.
> > 
> > Ideally there could be a stateless validateExecutorInfo, in addition to the 
> > one that needs the master state. In the absence of this, you can write a 
> > test that spins up a framework and launches a task with a negative shutdown 
> > duration.
> 
> Ben Mahler wrote:
> I see now that you tested this in https://reviews.apache.org/r/44994/ 
> inside a larger test, would have been helpful to mention that here since the 
> testing done section points to https://reviews.apache.org/r/44662 which seems 
> to be only a CHANGELOG change..?

I suggested on https://reviews.apache.org/r/44994/ that we pull out a smaller 
test for the validation and place it inside `master_validation_tests.cpp` to be 
more consistent.


- Ben


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


On March 17, 2016, 11:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44854/
> ---
> 
> (Updated March 17, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
> https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
> 
> Diff: https://reviews.apache.org/r/44854/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 44854: Added validation for executor's shutdown grace period.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov