You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Hitesh Khamesra <hk...@pivotal.io> on 2016/02/01 21:27:29 UTC
Review Request 43059: Updated ViewCreator logic and fixed couple of
issues
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
Repository: geode
Description
-------
ViewCreator thread sends another message when any member doesn't ack back to
prepared view. And then it waits on future as this can happen for multiple members.
In this case, If those members are not responsive and other thread already determined that,
then we don't need to wait for those members. Thus now viewCreator thread checks
RemoveMember message for those members while waiting for response.
Another minor fix in same area.
And added unit test for that
Diffs
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
Diff: https://reviews.apache.org/r/43059/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Feb. 2, 2016, 4:51 p.m., Bruce Schuchardt wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java, line 2164
> > <https://reviews.apache.org/r/43059/diff/2/?file=1228853#file1228853line2164>
> >
> > This block of code could be replaced with filterLeavingMembers like you did in the new code.
changed to new filterRequests function
> On Feb. 2, 2016, 4:51 p.m., Bruce Schuchardt wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java, line 2258
> > <https://reviews.apache.org/r/43059/diff/1-2/?file=1228606#file1228606line2258>
> >
> > Members who have sent a Leave request are healthy and should not be considered crashed, so they should not be put back into the mbrs collection. Only the Removed members that we haven't received a Leave from should be put back into mbrs.
> >
> > So, get rid of the if(!newLeaves.isEmpty) block and change the other to this:
> >
> > if (!newRemovals.isEmpty()) {
> > newRemovals.removeAll(newLeaves);
> > mbrs.addAll(newRemovals);
> > }
Thanks. I missed this line "mbrs.removeAll(newLeaves);"
> On Feb. 2, 2016, 4:51 p.m., Bruce Schuchardt wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java, line 2278
> > <https://reviews.apache.org/r/43059/diff/2/?file=1228853#file1228853line2278>
> >
> > "removalRequestForMembers" isn't a good name for this parameter since they might be Remove requests or Leave requests. Call it "requestIDs" or something without "remove" in the name.
changed to filterMembers
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117414
-----------------------------------------------------------
On Feb. 2, 2016, 12:11 a.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 12:11 a.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117414
-----------------------------------------------------------
That's a lot better
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2254)
<https://reviews.apache.org/r/43059/#comment178596>
Members who have sent a Leave request are healthy and should not be considered crashed, so they should not be put back into the mbrs collection. Only the Removed members that we haven't received a Leave from should be put back into mbrs.
So, get rid of the if(!newLeaves.isEmpty) block and change the other to this:
if (!newRemovals.isEmpty()) {
newRemovals.removeAll(newLeaves);
mbrs.addAll(newRemovals);
}
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2164)
<https://reviews.apache.org/r/43059/#comment178597>
This block of code could be replaced with filterLeavingMembers like you did in the new code.
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2265)
<https://reviews.apache.org/r/43059/#comment178598>
"removalRequestForMembers" isn't a good name for this parameter since they might be Remove requests or Leave requests. Call it "requestIDs" or something without "remove" in the name.
- Bruce Schuchardt
On Feb. 2, 2016, 12:11 a.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 12:11 a.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117318
-----------------------------------------------------------
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java (line 99)
<https://reviews.apache.org/r/43059/#comment178481>
Maybe instead of passing in a boolean for useTestGMSJoinLeave, instead just pass in the instance of GMSJoinLeave that you want to test with? The initMocks from above could then be changed to initMocks(enableNetworkPartition, new GMSJoinLeave()) and the call that needs to pass in true would just pass in the GMSJoinLeaveTest() instead.
- Jason Huynh
On Feb. 2, 2016, 12:11 a.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 12:11 a.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117440
-----------------------------------------------------------
If you can do something about that long sleep I think this will be ready for testing
- Bruce Schuchardt
On Feb. 2, 2016, 5:28 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 5:28 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117852
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Feb. 2, 2016, 11:10 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 11:10 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117525
-----------------------------------------------------------
Ship it!
Looks good. If I were going to suggest another change it would be to have checkIfAvailable return a boolean instead of a memberId/null.
- Bruce Schuchardt
On Feb. 2, 2016, 11:10 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 11:10 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/
-----------------------------------------------------------
(Updated Feb. 2, 2016, 11:10 p.m.)
Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
Changes
-------
Removed sleep and added notify logic
Repository: geode
Description
-------
ViewCreator thread sends another message when any member doesn't ack back to
prepared view. And then it waits on future as this can happen for multiple members.
In this case, If those members are not responsive and other thread already determined that,
then we don't need to wait for those members. Thus now viewCreator thread checks
RemoveMember message for those members while waiting for response.
Another minor fix in same area.
And added unit test for that
Diffs (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
Diff: https://reviews.apache.org/r/43059/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/
-----------------------------------------------------------
(Updated Feb. 2, 2016, 5:28 p.m.)
Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
Changes
-------
updated diff based on comments
Repository: geode
Description
-------
ViewCreator thread sends another message when any member doesn't ack back to
prepared view. And then it waits on future as this can happen for multiple members.
In this case, If those members are not responsive and other thread already determined that,
then we don't need to wait for those members. Thus now viewCreator thread checks
RemoveMember message for those members while waiting for response.
Another minor fix in same area.
And added unit test for that
Diffs (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
Diff: https://reviews.apache.org/r/43059/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/
-----------------------------------------------------------
(Updated Feb. 2, 2016, 12:11 a.m.)
Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
Repository: geode
Description
-------
ViewCreator thread sends another message when any member doesn't ack back to
prepared view. And then it waits on future as this can happen for multiple members.
In this case, If those members are not responsive and other thread already determined that,
then we don't need to wait for those members. Thus now viewCreator thread checks
RemoveMember message for those members while waiting for response.
Another minor fix in same area.
And added unit test for that
Diffs (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
Diff: https://reviews.apache.org/r/43059/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117302
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Feb. 1, 2016, 8:27 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2016, 8:27 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Feb. 1, 2016, 9:56 p.m., Bruce Schuchardt wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java, line 2273
> > <https://reviews.apache.org/r/43059/diff/1/?file=1228606#file1228606line2273>
> >
> > I think this should just break out of the for-loop and the newRemovals (minus newLeaves) should be added back into the mbrs collection when the loop completes.
Thanks Bruce. I have added both removal/leave request to mbrs after the while loop.
> On Feb. 1, 2016, 9:56 p.m., Bruce Schuchardt wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java, line 2282
> > <https://reviews.apache.org/r/43059/diff/1/?file=1228606#file1228606line2282>
> >
> > Since there is already a while() loop with a timer I think this sleep should be pretty short - on the order of the 100ms you're using for the other sleep.
I will look little bit more to avoid sleep.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117270
-----------------------------------------------------------
On Feb. 2, 2016, 12:11 a.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 12:11 a.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 43059: Updated ViewCreator logic and fixed couple
of issues
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43059/#review117270
-----------------------------------------------------------
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2206)
<https://reviews.apache.org/r/43059/#comment178430>
I think you can just reuse the newRemovals collection instead of introducing another one that has the same purpose.
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2224)
<https://reviews.apache.org/r/43059/#comment178424>
Let's get rid if this variable ("now"). It's only used in the while() expression. Then the while() expression can be simplified
while (System.currentTimeMillis() <= giveUpTime)
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2225)
<https://reviews.apache.org/r/43059/#comment178419>
remove debug log statement
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2243)
<https://reviews.apache.org/r/43059/#comment178431>
Here you should also look for new Leave requests, like was done before creating the futures. That will keep us from declaring a crash for a member that has actually sent a Leave request.
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2246)
<https://reviews.apache.org/r/43059/#comment178417>
remove or make debug-level
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2259)
<https://reviews.apache.org/r/43059/#comment178421>
I think this should just break out of the for-loop and the newRemovals (minus newLeaves) should be added back into the mbrs collection when the loop completes.
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2268)
<https://reviews.apache.org/r/43059/#comment178422>
Since there is already a while() loop with a timer I think this sleep should be pretty short - on the order of the 100ms you're using for the other sleep.
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2279)
<https://reviews.apache.org/r/43059/#comment178427>
This needs braces.
for () {
statements
}
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java (line 1115)
<https://reviews.apache.org/r/43059/#comment178428>
remove TODO
- Bruce Schuchardt
On Feb. 1, 2016, 8:27 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43059/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2016, 8:27 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ViewCreator thread sends another message when any member doesn't ack back to
> prepared view. And then it waits on future as this can happen for multiple members.
> In this case, If those members are not responsive and other thread already determined that,
> then we don't need to wait for those members. Thus now viewCreator thread checks
> RemoveMember message for those members while waiting for response.
> Another minor fix in same area.
> And added unit test for that
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b6f6c12
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java 6b09214
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java 5b53290
>
> Diff: https://reviews.apache.org/r/43059/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>