Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-10-04 Thread Alexander Rukletsov 

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


Ship it!




Ship It!

- Alexander Rukletsov


On Oct. 4, 2016, 1:18 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Oct. 4, 2016, 1:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-10-04 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [52357]

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

- Mesos ReviewBot


On Oct. 4, 2016, 1:18 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Oct. 4, 2016, 1:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-10-04 Thread Gastón Kleiman 

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

(Updated Oct. 4, 2016, 1:18 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Use a static name for the tmp file created by the new test in the suite's 
temporary directory.


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


Repository: mesos


Description
---

The health check library would ignore all failures until after the end
of the grace period.

This behaviour was misleading. With the changes in this commit, once a
health check succeeds for the first time, the grace period rule for
failures is not be applied any more.


Diffs (updated)
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 
  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---

Manual tests using Marathon.
Added a new unit test and verified that it fails without the change, but 
succeeds with it.


Thanks,

Gastón Kleiman

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-09-30 Thread Gastón Kleiman 

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

(Updated Sept. 30, 2016, 11:29 a.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Followed the suggestions from AlexR and Haosdent, which led to way fewer 
boilerplate in the test.


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


Repository: mesos


Description
---

The health check library would ignore all failures until after the end
of the grace period.

This behaviour was misleading. With the changes in this commit, once a
health check succeeds for the first time, the grace period rule for
failures is not be applied any more.


Diffs (updated)
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 
  src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 

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


Testing
---

Manual tests using Marathon.
Added a new unit test and verified that it fails without the change, but 
succeeds with it.


Thanks,

Gastón Kleiman

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-09-30 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [52357]

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

- Mesos ReviewBot


On Sept. 29, 2016, 3:30 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Sept. 29, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-09-29 Thread Alexander Rukletsov 

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




src/tests/health_check_tests.cpp (line 1333)


s/Testing/Tests

Let's repeat what we test and how.



src/tests/health_check_tests.cpp (lines 1339 - 1340)


I wonder whether we really need these lines.



src/tests/health_check_tests.cpp (line 1342)


This can also go.



src/tests/health_check_tests.cpp (line 1373)


Let's name it similar to test case.



src/tests/health_check_tests.cpp (line 1387)


s/alt/healthCheckCmd ?



src/tests/health_check_tests.cpp (lines 1401 - 1402)


.WillRepeatedly(Return()); // Ignore subsequent updates.


- Alexander Rukletsov


On Sept. 29, 2016, 3:30 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Sept. 29, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-09-29 Thread haosdent huang 

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




src/tests/health_check_tests.cpp (line 1336)


As @alexr metioned in https://reviews.apache.org/r/52301/#review150860

We could speed up this test case by

```
  master::Flags masterFlags = CreateMasterFlags();
  masterFlags.allocation_interval = Milliseconds(50);
  Try> master = StartMaster(masterFlags);
  ASSERT_SOME(master);
```

here.



src/tests/health_check_tests.cpp (lines 1344 - 1354)


And it is not need to create containerizer here.

We could use 

```
  slave::Flags flags = CreateSlaveFlags();
  flags.isolation = "posix/cpu,posix/mem";

  Owned detector = master.get()->createDetector();

  Try> agent = StartSlave(detector.get());
  ASSERT_SOME(agent);
```


- haosdent huang


On Sept. 29, 2016, 3:30 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Sept. 29, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-09-29 Thread Gastón Kleiman 

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

(Updated Sept. 29, 2016, 3:30 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Addressed haosdent's comments.


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


Repository: mesos


Description
---

The health check library would ignore all failures until after the end
of the grace period.

This behaviour was misleading. With the changes in this commit, once a
health check succeeds for the first time, the grace period rule for
failures is not be applied any more.


Diffs (updated)
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 
  src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 

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


Testing
---

Manual tests using Marathon.
Added a new unit test and verified that it fails without the change, but 
succeeds with it.


Thanks,

Gastón Kleiman

Re: Review Request 52357: Improved handling of health check failures within the grace period.

2016-09-28 Thread haosdent huang 

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


Fix it, then Ship it!




Ship It!


src/tests/health_check_tests.cpp (lines 1373 - 1375)


Let's use 


os::mktemp(path::join(os::getcwd(), "XX"));
```

here and it would be clean automatically when test cases tear down.

So that we could avoid leak files in `/tmp`. 

I think `HealthCheckTest.HealthStatusChange` need to be updated as well, 
but let's do it in a separate patch.



src/tests/health_check_tests.cpp (lines 1389 - 1390)


It would be better that we add a comment said `` is the grace_period. 
Or use `createTask()` instead of `populateTasks`.



src/tests/health_check_tests.cpp (lines 1414 - 1415)


Could remove this if we use 

```
os::mktemp(path::join(os::getcwd(), "XX"));
```


- haosdent huang


On Sept. 28, 2016, 5 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Sept. 28, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 52357: Improved handling of health check failures within the grace period.

2016-09-28 Thread Gastón Kleiman 

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

Review request for mesos, Alexander Rukletsov and haosdent huang.


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


Repository: mesos


Description
---

The health check library would ignore all failures until after the end
of the grace period.

This behaviour was misleading. With the changes in this commit, once a
health check succeeds for the first time, the grace period rule for
failures is not be applied any more.


Diffs
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 
  src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 

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


Testing
---

Manual tests using Marathon.
Added a new unit test and verified that it fails without the change, but 
succeeds with it.


Thanks,

Gastón Kleiman