GEODE-8579: Stop waiting locator-wait-time if all locators are available #5598
Conversation
If we can contact all other locators, we should stop waiting for locator-wait-time to elapse.
upthewaterspout
requested review from
Bill,
aaronlindsey,
bschuchardt and
rhoughton-pivot
|
BTW, the actual product change is just shifting some parenthesis around in an if statement. Most of these changes have to do with moving AvailablePort so that I could write a good test. We need to be able to write a test at the membership level where two locators are started up that both know about each other (including their ports) at startup. The best way to do that is to use AvailablePort to get the ports before starting the locators. |
| @@ -57,7 +56,7 @@ | |||
| @BeforeClass | |||
| public static void startLocator() throws IOException { | |||
| rootPath = gfshRule.getTemporaryFolder().getRoot().toPath(); | |||
| locatorPort = getRandomAvailablePort(SOCKET); | |||
| locatorPort = AvailablePortHelper.getRandomAvailableTCPPort(); | |||
| @@ -32,7 +32,7 @@ | |||
| import java.util.Random; | |||
|
|
|||
| import org.apache.geode.annotations.Immutable; | |||
| import org.apache.geode.distributed.internal.DistributionConfig; | |||
| import org.apache.geode.distributed.internal.membership.api.MembershipConfig; | |||
| // Make sure the members see each other in the view | ||
| assertThat(membership0.getView().getMembers()).hasSize(2); | ||
| assertThat(membership1.getView().getMembers()).hasSize(2); | ||
| } |
There was a problem hiding this comment.
This is a good test and it's acceptable as-is. I wonder though, if you'd be willing to entertain reducing run-time and increasing determinism by by injecting the (millisecond) time keeper and sleeper into the locator, the way it's done for PrimaryHandler?
Here is the PR from August that added that injection: #5422
As of that PR (and now on develop) there are a couple functional interfaces defined in PrimaryHander that could be hoisted a little higher and used in MembershipLocator for your purposes, i.e. Sleeper, MillisecondProvider.
With those injected, this test could control time similar to how PrimaryHandlerTest does.
There was a problem hiding this comment.
Based on your comment I started down this road. But this is a test of the whole membership system, so this will require injecting virtual time into a whole lot of classes, including classes that are doing things like wait(sometimeout) etc. I feel like this needs a little bit more time and careful development to get right, and more than I want to shoehorn into this PR.
There was a problem hiding this comment.
Ok @upthewaterspout. After this PR merges I'll write a ticket to make this improvement. Thanks for trying it though!
There was a problem hiding this comment.
Here's that new ticket: https://issues.apache.org/jira/browse/GEODE-8589
| e.printStackTrace(); | ||
| ExitCode.FATAL.doSystemExit(); | ||
| } | ||
| addr = InetAddress.getByName(addrString); |
There was a problem hiding this comment.
verified when address cannot be resolved the exception causes exit code 1 (on macos) ✓
.../main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
Outdated
Show resolved
Hide resolved
…stem We should only become the coordinator if we have not contacted any joined members.
AvailablePort moved, so is no longer needed as a sanctioned class in this test.
upthewaterspout
deleted the
feature/stop-waiting-if-all-locators-contacted
branch
If we can contact all other locators, we should stop waiting for
locator-wait-time to elapse.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.