You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/04/21 19:36:42 UTC
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7554: GEM-3124: Guarding membership addition code paths to omit membership …
pivotal-jbarrett commented on code in PR #7554:
URL: https://github.com/apache/geode/pull/7554#discussion_r855531377
##########
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java:
##########
@@ -227,7 +248,9 @@ public class ClusterDistributionManager implements DistributionManager {
private Map<InternalDistributedMember, Collection<String>> hostedLocatorsWithSharedConfiguration =
Collections.emptyMap();
- /** a map keyed on InternalDistributedMember, to direct channels to other systems */
+ /**
+ * a map keyed on InternalDistributedMember, to direct channels to other systems
+ */
// protected final Map channelMap = CFactory.createCM();
Review Comment:
Cleanup of javadoc on a commented out member, please delete the member and javadoc.
##########
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java:
##########
@@ -353,7 +380,9 @@ static ClusterDistributionManager create(InternalDistributedSystem system,
}
}
}
+ // if (!system.getDistributionManager().getViewMembers().contains(id)) {
Review Comment:
Delete commented out code.
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/cache/persistence/MissingDiskStoreAcceptanceTest.java:
##########
@@ -70,40 +70,60 @@ public void setUp() throws Exception {
server1Folder = temporaryFolder.newFolder(SERVER_1_NAME).toPath().toAbsolutePath();
server2Folder = temporaryFolder.newFolder(SERVER_2_NAME).toPath().toAbsolutePath();
- int[] ports = getRandomAvailableTCPPorts(3);
+ int[] ports = getRandomAvailableTCPPorts(9);
locatorPort = ports[0];
int server1Port = ports[1];
int server2Port = ports[2];
+ int httpPort1 = ports[3];
+ int httpPort2 = ports[4];
+ int httpPort3 = ports[5];
+ int jmxPort1 = ports[6];
+ int jmxPort2 = ports[7];
+ int jmxPort3 = ports[8];
String startLocatorCommand = String.join(" ",
"start locator",
"--name=" + LOCATOR_NAME,
"--dir=" + locatorFolder,
"--port=" + locatorPort,
+ "--http-service-port=" + httpPort1,
+ "--J=-Dgemfire.jmx-manager-port=" + jmxPort1,
"--locators=localhost[" + locatorPort + "]");
startServer1Command = String.join(" ",
"start server",
"--name=" + SERVER_1_NAME,
"--dir=" + server1Folder,
"--locators=localhost[" + locatorPort + "]",
+ "--http-service-port=" + httpPort2,
+ "--J=-Dgemfire.jmx-manager-port=" + jmxPort2,
"--server-port=" + server1Port);
startServer2Command = String.join(" ",
"start server",
"--name=" + SERVER_2_NAME,
"--dir=" + server2Folder,
"--locators=localhost[" + locatorPort + "]",
+ "--http-service-port=" + httpPort3,
Review Comment:
Does this test really need to start the http service on the servers?
##########
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java:
##########
@@ -958,13 +986,13 @@ public Collection<String> getHostedLocators(InternalDistributedMember member) {
* Returns a copy of the map of all members hosting locators. The key is the member, and the value
* is a collection of host[port] strings. If a bind-address was used for a locator then the form
* is bind-addr[port].
- *
+ * <p>
Review Comment:
Is it possible this PR is conflicting with one recently to cleanup Javadocs?
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/cache/persistence/MissingDiskStoreAcceptanceTest.java:
##########
@@ -70,40 +70,60 @@ public void setUp() throws Exception {
server1Folder = temporaryFolder.newFolder(SERVER_1_NAME).toPath().toAbsolutePath();
server2Folder = temporaryFolder.newFolder(SERVER_2_NAME).toPath().toAbsolutePath();
- int[] ports = getRandomAvailableTCPPorts(3);
+ int[] ports = getRandomAvailableTCPPorts(9);
locatorPort = ports[0];
int server1Port = ports[1];
int server2Port = ports[2];
+ int httpPort1 = ports[3];
+ int httpPort2 = ports[4];
+ int httpPort3 = ports[5];
+ int jmxPort1 = ports[6];
+ int jmxPort2 = ports[7];
+ int jmxPort3 = ports[8];
String startLocatorCommand = String.join(" ",
"start locator",
"--name=" + LOCATOR_NAME,
"--dir=" + locatorFolder,
"--port=" + locatorPort,
+ "--http-service-port=" + httpPort1,
+ "--J=-Dgemfire.jmx-manager-port=" + jmxPort1,
"--locators=localhost[" + locatorPort + "]");
startServer1Command = String.join(" ",
"start server",
"--name=" + SERVER_1_NAME,
"--dir=" + server1Folder,
"--locators=localhost[" + locatorPort + "]",
+ "--http-service-port=" + httpPort2,
+ "--J=-Dgemfire.jmx-manager-port=" + jmxPort2,
"--server-port=" + server1Port);
startServer2Command = String.join(" ",
"start server",
"--name=" + SERVER_2_NAME,
"--dir=" + server2Folder,
"--locators=localhost[" + locatorPort + "]",
+ "--http-service-port=" + httpPort3,
+ "--J=-Dgemfire.jmx-manager-port=" + jmxPort3,
"--server-port=" + server2Port);
String createRegionCommand = String.join(" ",
"create region",
"--name=" + REGION_NAME,
"--type=REPLICATE_PERSISTENT");
- gfshRule.execute(startLocatorCommand, startServer1Command, startServer2Command,
- createRegionCommand);
+ gfshRule.execute(startLocatorCommand);
+ gfshRule.execute(startServer1Command, startServer2Command);
+ Thread.sleep(10000);
Review Comment:
Would be better to `await` until we can assert that the server is started. Arbitrary sleeps make for slower and flakier tests.
##########
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java:
##########
@@ -2373,7 +2398,7 @@ public void memberDeparted(InternalDistributedMember theId, boolean crashed, Str
message.setCrashed(crashed);
message.setAlertListenerExpected(true);
message.setIgnoreAlertListenerRemovalFailure(true); // we don't know if it was a listener
- // so
+ // so
Review Comment:
Cleanup wacky comment format please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org