You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jianxia Chen <jc...@pivotal.io> on 2016/03/07 18:45:55 UTC
Review Request 44458: Fix GEODE-1039 CI Failure:
GMSHealthMonitorJUnitTest.testCheckIfAvailableWithSimulatedHeartBeatWithTcpCheck
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44458/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
Repository: geode
Description
-------
It is still possible that during checkIfAvailable the time stamp of the heartbeat response is the same as the checkIfAvailable startTime. That's why the unit test fails.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java 536e26e
Diff: https://reviews.apache.org/r/44458/diff/
Testing
-------
Thanks,
Jianxia Chen
Re: Review Request 44458: Fix GEODE-1039 CI Failure:
GMSHealthMonitorJUnitTest.testCheckIfAvailableWithSimulatedHeartBeatWithTcpCheck
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44458/#review122419
-----------------------------------------------------------
Ship it!
Ship It!
- Hitesh Khamesra
On March 7, 2016, 8:14 p.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44458/
> -----------------------------------------------------------
>
> (Updated March 7, 2016, 8:14 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> It is still possible that during checkIfAvailable the time stamp of the heartbeat response is the same as the checkIfAvailable startTime. That's why the unit test fails.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java 536e26e
> geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitorJUnitTest.java a96b546
>
> Diff: https://reviews.apache.org/r/44458/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44458: Fix GEODE-1039 CI Failure:
GMSHealthMonitorJUnitTest.testCheckIfAvailableWithSimulatedHeartBeatWithTcpCheck
Posted by Jianxia Chen <jc...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44458/
-----------------------------------------------------------
(Updated March 7, 2016, 8:14 p.m.)
Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
1. Changed the test. Make sure checkIfAvailalbe happens before heartbeat response processing.
2. Change the health monitor code, "<=" becomes "<" in checkIfAvailable. Reason, heartbeat response processing could be fast enough, so that startTime == ts. Another option is, if we don't want to change the product code, enforce a slight delay (e.g. Thread.sleep) before processing the hearbeat response. This can be done by adding a sleep in doTCPCheckMember in the test.
Repository: geode
Description
-------
It is still possible that during checkIfAvailable the time stamp of the heartbeat response is the same as the checkIfAvailable startTime. That's why the unit test fails.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java 536e26e
geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitorJUnitTest.java a96b546
Diff: https://reviews.apache.org/r/44458/diff/
Testing
-------
Thanks,
Jianxia Chen
Re: Review Request 44458: Fix GEODE-1039 CI Failure:
GMSHealthMonitorJUnitTest.testCheckIfAvailableWithSimulatedHeartBeatWithTcpCheck
Posted by Jianxia Chen <jc...@pivotal.io>.
> On March 7, 2016, 6:28 p.m., Hitesh Khamesra wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java, line 1281
> > <https://reviews.apache.org/r/44458/diff/1/?file=1283187#file1283187line1281>
> >
> > We need to update test here as we send/simulate heartbeat before calling checkiFAvailable.
There is a tricky timing issue here. Ideally, in order for the test to pass, checkIfAvailable has to be called first, in order to get a value for startTime. But before it finishes, more specifically, before it reaches the line 1281 above, the heartbeat response should have been processed, so that ts has some value. Ideally, ts should be greater than startTime, because heartbeat response is processed after checkeIfAvaialble get a startTime value. However, even in this situation, the processing of heartbeat response could be fast enough that ts is still the same as startTime, causing a test failure. Unless we force a delay (e.g. Thread.sleep) in heartbeat response processing.
- Jianxia
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44458/#review122331
-----------------------------------------------------------
On March 7, 2016, 5:45 p.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44458/
> -----------------------------------------------------------
>
> (Updated March 7, 2016, 5:45 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> It is still possible that during checkIfAvailable the time stamp of the heartbeat response is the same as the checkIfAvailable startTime. That's why the unit test fails.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java 536e26e
>
> Diff: https://reviews.apache.org/r/44458/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44458: Fix GEODE-1039 CI Failure:
GMSHealthMonitorJUnitTest.testCheckIfAvailableWithSimulatedHeartBeatWithTcpCheck
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44458/#review122331
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java (line 1281)
<https://reviews.apache.org/r/44458/#comment184285>
We need to update test here as we send/simulate heartbeat before calling checkiFAvailable.
- Hitesh Khamesra
On March 7, 2016, 5:45 p.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44458/
> -----------------------------------------------------------
>
> (Updated March 7, 2016, 5:45 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> It is still possible that during checkIfAvailable the time stamp of the heartbeat response is the same as the checkIfAvailable startTime. That's why the unit test fails.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java 536e26e
>
> Diff: https://reviews.apache.org/r/44458/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>