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/07 22:32:57 UTC
[GitHub] [geode] kirklund opened a new pull request, #7571: [DRAFT] Add orphan prevention to GfshRule
kirklund opened a new pull request, #7571:
URL: https://github.com/apache/geode/pull/7571
Cleanup GfshRule, extract some methods back to tests
Save log files for failed acceptance tests
Kill geode processes that were not responsive to stop
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: [DRAFT] Add orphan prevention to GfshRule
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r869685152
##########
geode-junit/src/main/java/org/apache/geode/test/junit/rules/Folder.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.test.junit.rules;
+
+import static java.util.Objects.requireNonNull;
+import static org.apache.commons.io.FileUtils.deleteDirectory;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+public class Folder {
+
+ private final Path root;
+
+ public Folder(Path root) throws IOException {
+ requireNonNull(root);
+ this.root = Files.createDirectories(root.normalize().toAbsolutePath());
Review Comment:
I've fixed this one, but I think I might have this snippet repeated somewhere else in the PR.
--
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
[GitHub] [geode] demery-pivotal commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r879668886
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/ssl/CertificateRotationTest.java:
##########
@@ -339,16 +348,16 @@ private void startCluster() throws IOException, GeneralSecurityException {
.sanIpAddress(InetAddress.getByName("127.0.0.1"))
.generate();
- clusterKeyStore = temporaryFolder.newFile("cluster-keystore.jks");
- writeCertsToKeyStore(clusterKeyStore.toPath(), clusterCert);
+ clusterKeyStore = createFile(rootFolder.resolve("cluster-keystore.jks"));
+ writeCertsToKeyStore(clusterKeyStore, clusterCert);
- clusterTrustStore = temporaryFolder.newFile("cluster-truststore.jks");
- writeCertsToTrustStore(clusterTrustStore.toPath(), caCert);
+ clusterTrustStore = createFile(rootFolder.resolve("cluster-truststore.jks"));
+ writeCertsToTrustStore(clusterTrustStore, caCert);
- clusterSecurityProperties = temporaryFolder.newFile("cluster-security.properties");
+ clusterSecurityProperties = createFile(rootFolder.resolve("cluster-security.properties"));
Properties properties = CertStores.propertiesWith("all", "any", "any",
clusterTrustStore, dummyStorePass, clusterKeyStore, dummyStorePass, true, true);
- properties.store(new FileOutputStream(clusterSecurityProperties), "");
+ properties.store(new FileOutputStream(clusterSecurityProperties.toFile()), "");
Review Comment:
Maybe my theory was wrong, and that unclosed file stream wasn't the problem after all.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878596145
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/metrics/RegionEntriesGaugeTest.java:
##########
@@ -156,7 +158,8 @@ public void regionEntriesGaugeShowsCountOfLocalRegionValuesInServer() {
await()
.untilAsserted(() -> {
Review Comment:
Reformat this block.
--
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
[GitHub] [geode] demery-pivotal commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878598788
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/ssl/CertificateRotationTest.java:
##########
@@ -339,16 +348,16 @@ private void startCluster() throws IOException, GeneralSecurityException {
.sanIpAddress(InetAddress.getByName("127.0.0.1"))
.generate();
- clusterKeyStore = temporaryFolder.newFile("cluster-keystore.jks");
- writeCertsToKeyStore(clusterKeyStore.toPath(), clusterCert);
+ clusterKeyStore = createFile(rootFolder.resolve("cluster-keystore.jks"));
+ writeCertsToKeyStore(clusterKeyStore, clusterCert);
- clusterTrustStore = temporaryFolder.newFile("cluster-truststore.jks");
- writeCertsToTrustStore(clusterTrustStore.toPath(), caCert);
+ clusterTrustStore = createFile(rootFolder.resolve("cluster-truststore.jks"));
+ writeCertsToTrustStore(clusterTrustStore, caCert);
- clusterSecurityProperties = temporaryFolder.newFile("cluster-security.properties");
+ clusterSecurityProperties = createFile(rootFolder.resolve("cluster-security.properties"));
Properties properties = CertStores.propertiesWith("all", "any", "any",
clusterTrustStore, dummyStorePass, clusterKeyStore, dummyStorePass, true, true);
- properties.store(new FileOutputStream(clusterSecurityProperties), "");
+ properties.store(new FileOutputStream(clusterSecurityProperties.toFile()), "");
Review Comment:
That's written under a `TemporaryFolder`, which tries to delete its contents, but shrugs if it fails.
--
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
[GitHub] [geode] Bill commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
Bill commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r879954008
##########
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java:
##########
@@ -333,13 +333,15 @@ private ServerLauncher(final Builder builder) {
controlHandler = new ControlNotificationHandler() {
@Override
public void handleStop() {
+ logger.debug("Handling ControllableProcess stop request.");
Review Comment:
@kirklund what's the value of these two new debug-level log messages?
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r880024482
##########
geode-junit/src/main/java/org/apache/geode/test/version/TestVersion.java:
##########
@@ -17,18 +17,24 @@
import java.io.Serializable;
import java.util.Objects;
+@SuppressWarnings("serial")
public class TestVersion implements Comparable<TestVersion>, Serializable {
- public static final TestVersion CURRENT_VERSION = new TestVersion(VersionManager.CURRENT_VERSION);
+
+ static final TestVersion CURRENT_VERSION = new TestVersion(VersionManager.CURRENT_VERSION);
private final int major;
private final int minor;
private final int release;
+ public static TestVersion current() {
+ return CURRENT_VERSION;
+ }
+
Review Comment:
Thanks! That was the intention but it looks like changing VmConfiguration was skipped somehow.
--
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
[GitHub] [geode] demery-pivotal commented on a diff in pull request #7571: [DRAFT] Add orphan prevention to GfshRule
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r869678386
##########
geode-junit/src/main/java/org/apache/geode/test/junit/rules/Folder.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.test.junit.rules;
+
+import static java.util.Objects.requireNonNull;
+import static org.apache.commons.io.FileUtils.deleteDirectory;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+public class Folder {
+
+ private final Path root;
+
+ public Folder(Path root) throws IOException {
+ requireNonNull(root);
+ this.root = Files.createDirectories(root.normalize().toAbsolutePath());
Review Comment:
This should probably be `root.toAbsolutePath().normalize()`.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: [DRAFT] Add orphan prevention to GfshRule
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r874030269
##########
geode-junit/src/main/java/org/apache/geode/test/junit/rules/gfsh/GfshExecutor.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.test.junit.rules.gfsh;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeoutException;
+
+public interface GfshExecutor {
Review Comment:
`GfshScript` also have several `execute` methods but with different signatures.
The implementations of each rule's `execute` methods are too different to have any common code that could be extracted. However, all `execute` calls do eventually route to a single `execute` method in `GfshContext` that does all of the actual work. The "in-betweeners" just add in more parameters from fields or convert certain parameters such as `File` vs `Path`.
--
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
[GitHub] [geode] demery-pivotal commented on a diff in pull request #7571: [DRAFT] Add orphan prevention to GfshRule
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r869676436
##########
geode-assembly/src/upgradeTest/java/org/apache/geode/test/junit/rules/GfshContextVersionTest.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.test.junit.rules;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.internal.util.ProductVersionUtil;
+import org.apache.geode.test.junit.categories.GfshTest;
+import org.apache.geode.test.junit.rules.gfsh.GfshExecutor;
+import org.apache.geode.test.junit.rules.gfsh.GfshRule;
+
+@Category(GfshTest.class)
+public class GfshContextVersionTest {
Review Comment:
Should this be `GfshExecutorVersionTest`?
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: [DRAFT] Add orphan prevention to GfshRule
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r869604871
##########
geode-core/src/upgradeTest/java/org/apache/geode/internal/net/SocketCreatorUpgradeTest.java:
##########
@@ -244,26 +264,33 @@ public void upgradingToNewGeodeOnOldJavaWithProtocolsTLSv1_2() throws IOExceptio
}
@Test
- public void upgradingToNewGeodeOnOldJavaWithProtocolsTLSv1_2Hangs() throws IOException {
- assumeThat(version).as("Geode 1.12.0 and older can upgrade.")
+ public void upgradingToNewGeodeOnOldJavaWithProtocolsTLSv1_2Hangs()
+ throws IOException, ExecutionException, InterruptedException, TimeoutException {
+ assumeThat(testVersion).as("Geode 1.12.0 and older can upgrade.")
.isGreaterThan(TestVersion.valueOf("1.12.0"));
- assumeThat(version).as("Geode between [1.12.1, 1.3.0) can't connect p2p with just TLSv1.2")
+ assumeThat(testVersion).as("Geode between [1.12.1, 1.3.0) can't connect p2p with just TLSv1.2")
.isGreaterThanOrEqualTo(TestVersion.valueOf("1.13.0"));
generateSecurityProperties(PROTOCOL_TLSv1_2, securityPropertiesFile, keyStoreFile,
trustStoreFile);
gfshOldGeodeOldJava.execute(root, startLocator1);
- runAsync(() -> gfshNewGeodeOldJava.execute(root, startLocator2));
+ runAsync(() -> {
+ try {
+ gfshNewGeodeOldJava.execute(root, startLocator2);
+ } catch (IOException | ExecutionException | InterruptedException | TimeoutException ignore) {
+ // ignored
+ }
+ });
Review Comment:
This is verbose and ugly. Consider wrapping checked exceptions inside an unchecked exception.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878590276
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/launchers/ServerStartupOnlineTest.java:
##########
@@ -116,13 +114,14 @@ public void startServerReturnsAfterStartupTaskCompletes() throws Exception {
}
@Test
- public void statusServerReportsStartingUntilStartupTaskCompletes() throws Exception {
+ public void statusServerReportsStartingUntilStartupTaskCompletes()
+ throws InterruptedException, IOException {
CompletableFuture<Void> startServerTask =
executorServiceRule.runAsync(() -> gfshRule.execute(startServerCommand));
waitForStartServerCommandToHang();
- await().untilAsserted(() -> {
+ await().atMost(Duration.ofMinutes(1)).untilAsserted(() -> {
Review Comment:
Remove `atMost` and use default.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: [DRAFT] Add orphan prevention to GfshRule
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r869597915
##########
geode-assembly/src/upgradeTest/java/org/apache/geode/session/tests/Tomcat8ClientServerRollingUpgradeTest.java:
##########
@@ -125,21 +130,30 @@ protected void startLocator(String name, String classPath, int port, GfshRule gf
@Before
public void setup() throws Exception {
+ Path tempFolder = folderRule.getFolder().toPath();
+ currentGfsh = gfshRule.executor()
+ .withGfshJvmOptions("-Dgfsh.log-level=fine")
+ .build(tempFolder);
+ oldGfsh = gfshRule.executor()
+ .withGfshJvmOptions("-Dgfsh.log-level=fine")
+ .withGeodeVersion(oldVersion)
+ .build(tempFolder);
Review Comment:
Remove temporary debug code.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878598310
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/ssl/CertificateRotationTest.java:
##########
@@ -339,16 +348,16 @@ private void startCluster() throws IOException, GeneralSecurityException {
.sanIpAddress(InetAddress.getByName("127.0.0.1"))
.generate();
- clusterKeyStore = temporaryFolder.newFile("cluster-keystore.jks");
- writeCertsToKeyStore(clusterKeyStore.toPath(), clusterCert);
+ clusterKeyStore = createFile(rootFolder.resolve("cluster-keystore.jks"));
+ writeCertsToKeyStore(clusterKeyStore, clusterCert);
- clusterTrustStore = temporaryFolder.newFile("cluster-truststore.jks");
- writeCertsToTrustStore(clusterTrustStore.toPath(), caCert);
+ clusterTrustStore = createFile(rootFolder.resolve("cluster-truststore.jks"));
+ writeCertsToTrustStore(clusterTrustStore, caCert);
- clusterSecurityProperties = temporaryFolder.newFile("cluster-security.properties");
+ clusterSecurityProperties = createFile(rootFolder.resolve("cluster-security.properties"));
Properties properties = CertStores.propertiesWith("all", "any", "any",
clusterTrustStore, dummyStorePass, clusterKeyStore, dummyStorePass, true, true);
- properties.store(new FileOutputStream(clusterSecurityProperties), "");
+ properties.store(new FileOutputStream(clusterSecurityProperties.toFile()), "");
Review Comment:
Close file stream? Why does this one pass on Windows?
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878595675
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/metrics/GatewayReceiverMetricsTest.java:
##########
@@ -178,7 +178,8 @@ public void stopClusters() {
String stopReceiverLocatorCommand = "stop locator --dir=" + receiverLocatorFolder;
String stopSenderLocatorCommand = "stop locator --dir=" + senderLocatorFolder;
- gfshRule.execute(stopReceiverServerCommand, stopSenderServerCommand, stopReceiverLocatorCommand,
+ gfshRule.execute(stopReceiverServerCommand, stopSenderServerCommand,
+ stopReceiverLocatorCommand,
Review Comment:
Fix linewrap.
--
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
[GitHub] [geode] kirklund commented on pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on PR #7571:
URL: https://github.com/apache/geode/pull/7571#issuecomment-1137655729
One precheckin run hit [GEODE-10332](https://issues.apache.org/jira/browse/GEODE-10332) (ClientAuthenticationDUnitTest) and it was determined to be unrelated to this PR.
That same precheckin also hit [GEODE-10333](https://issues.apache.org/jira/browse/GEODE-10333) (WANRollingUpgradeNewSenderProcessOldEvent) and it was also determined to be unrelated to this PR.
Both failures involve membership layer and appear to involve port configuration issues in these two tests.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r880025107
##########
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java:
##########
@@ -333,13 +333,15 @@ private ServerLauncher(final Builder builder) {
controlHandler = new ControlNotificationHandler() {
@Override
public void handleStop() {
+ logger.debug("Handling ControllableProcess stop request.");
Review Comment:
Honestly I think these two debug statements should be removed. They had some value while debugging acceptance tests in Java 17 but keeping it doesn't seem valuable.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878594421
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/management/internal/rest/StandaloneClientManagementAPIAcceptanceTest.java:
##########
@@ -69,72 +65,94 @@ public static Collection<Boolean> data() {
@Parameter
public Boolean useSsl;
+ private String trustStorePath;
private ProcessLogger clientProcessLogger;
+ private Path rootFolder;
+
+ @Rule(order = 0)
+ public FolderRule folderRule = new FolderRule();
+ @Rule(order = 1)
+ public GfshRule gfshRule = new GfshRule(folderRule::getFolder);
+
+ @Before
+ public void setUp() {
+ rootFolder = folderRule.getFolder().toPath();
- @BeforeClass
- public static void beforeClass() {
/*
* This file was generated with:
* keytool -genkey -dname "CN=localhost" -alias self -validity 3650 -keyalg EC \
* -keystore trusted.keystore -keypass password -storepass password \
* -ext san=ip:127.0.0.1,dns:localhost -storetype jks
*/
- trustStorePath =
- createTempFileFromResource(StandaloneClientManagementAPIAcceptanceTest.class,
- "/ssl/trusted.keystore").getAbsolutePath();
- assertThat(trustStorePath).as("java file resource not found").isNotBlank();
+ trustStorePath = createTempFileFromResource(
+ StandaloneClientManagementAPIAcceptanceTest.class, "/ssl/trusted.keystore")
+ .getAbsolutePath();
+ assertThat(trustStorePath)
+ .as("java file resource not found")
+ .isNotBlank();
}
@After
- public void tearDown() throws Exception {
- clientProcessLogger.awaitTermination(GeodeAwaitility.getTimeout().toMillis(), MILLISECONDS);
+ public void tearDown() throws InterruptedException, ExecutionException, TimeoutException {
+ clientProcessLogger.awaitTermination(getTimeout().toMillis(), MILLISECONDS);
clientProcessLogger.close();
}
@Test
- public void clientCreatesRegionUsingClusterManagementService() throws Exception {
+ public void clientCreatesRegionUsingClusterManagementService()
+ throws IOException, InterruptedException {
JarBuilder jarBuilder = new JarBuilder();
String filePath =
createTempFileFromResource(getClass(), "/ManagementClientCreateRegion.java")
.getAbsolutePath();
assertThat(filePath).as("java file resource not found").isNotBlank();
- File outputJar = new File(tempDir.getRoot(), "output.jar");
+ File outputJar = new File(rootFolder.toFile(), "output.jar");
jarBuilder.buildJar(outputJar, new File(filePath));
- int[] availablePorts = AvailablePortHelper.getRandomAvailableTCPPorts(3);
+ int[] availablePorts = getRandomAvailableTCPPorts(3);
int locatorPort = availablePorts[0];
int httpPort = availablePorts[1];
int jmxPort = availablePorts[2];
GfshExecution startCluster =
- GfshScript.of(
- String.format(
- "start locator --port=%d --http-service-port=%d --J=-Dgemfire.JMX_MANAGER_PORT=%d %s",
- locatorPort, httpPort, jmxPort, getSslParameters()),
- String.format("start server --locators=localhost[%d] --server-port=0", locatorPort))
- .withName("startCluster").execute(gfsh);
-
+ GfshScript
Review Comment:
Optionally reformat block to match similar blocks of code.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878594588
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/management/internal/rest/StandaloneClientManagementAPIAcceptanceTest.java:
##########
@@ -69,72 +65,94 @@ public static Collection<Boolean> data() {
@Parameter
public Boolean useSsl;
+ private String trustStorePath;
private ProcessLogger clientProcessLogger;
+ private Path rootFolder;
+
+ @Rule(order = 0)
+ public FolderRule folderRule = new FolderRule();
+ @Rule(order = 1)
+ public GfshRule gfshRule = new GfshRule(folderRule::getFolder);
+
+ @Before
+ public void setUp() {
+ rootFolder = folderRule.getFolder().toPath();
- @BeforeClass
- public static void beforeClass() {
/*
* This file was generated with:
* keytool -genkey -dname "CN=localhost" -alias self -validity 3650 -keyalg EC \
* -keystore trusted.keystore -keypass password -storepass password \
* -ext san=ip:127.0.0.1,dns:localhost -storetype jks
*/
- trustStorePath =
- createTempFileFromResource(StandaloneClientManagementAPIAcceptanceTest.class,
- "/ssl/trusted.keystore").getAbsolutePath();
- assertThat(trustStorePath).as("java file resource not found").isNotBlank();
+ trustStorePath = createTempFileFromResource(
+ StandaloneClientManagementAPIAcceptanceTest.class, "/ssl/trusted.keystore")
+ .getAbsolutePath();
+ assertThat(trustStorePath)
+ .as("java file resource not found")
+ .isNotBlank();
}
@After
- public void tearDown() throws Exception {
- clientProcessLogger.awaitTermination(GeodeAwaitility.getTimeout().toMillis(), MILLISECONDS);
+ public void tearDown() throws InterruptedException, ExecutionException, TimeoutException {
+ clientProcessLogger.awaitTermination(getTimeout().toMillis(), MILLISECONDS);
clientProcessLogger.close();
}
@Test
- public void clientCreatesRegionUsingClusterManagementService() throws Exception {
+ public void clientCreatesRegionUsingClusterManagementService()
+ throws IOException, InterruptedException {
JarBuilder jarBuilder = new JarBuilder();
String filePath =
createTempFileFromResource(getClass(), "/ManagementClientCreateRegion.java")
.getAbsolutePath();
assertThat(filePath).as("java file resource not found").isNotBlank();
- File outputJar = new File(tempDir.getRoot(), "output.jar");
+ File outputJar = new File(rootFolder.toFile(), "output.jar");
jarBuilder.buildJar(outputJar, new File(filePath));
- int[] availablePorts = AvailablePortHelper.getRandomAvailableTCPPorts(3);
+ int[] availablePorts = getRandomAvailableTCPPorts(3);
int locatorPort = availablePorts[0];
int httpPort = availablePorts[1];
int jmxPort = availablePorts[2];
GfshExecution startCluster =
- GfshScript.of(
- String.format(
- "start locator --port=%d --http-service-port=%d --J=-Dgemfire.JMX_MANAGER_PORT=%d %s",
- locatorPort, httpPort, jmxPort, getSslParameters()),
- String.format("start server --locators=localhost[%d] --server-port=0", locatorPort))
- .withName("startCluster").execute(gfsh);
-
+ GfshScript
+ .of(
+ String.format(
+ "start locator --port=%d --http-service-port=%d --J=-Dgemfire.JMX_MANAGER_PORT=%d %s",
+ locatorPort, httpPort, jmxPort, getSslParameters()),
+ String.format("start server --locators=localhost[%d] --server-port=0", locatorPort))
+ .withName("startCluster")
+ .execute(gfshRule);
assertThat(startCluster.getProcess().exitValue())
- .as("Cluster did not start correctly").isEqualTo(0);
+ .as("Cluster did not start correctly")
+ .isEqualTo(0);
Process process = launchClientProcess(outputJar, httpPort);
boolean exited = process.waitFor(30, TimeUnit.SECONDS);
Review Comment:
Change this to use the default timeout.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r879692658
##########
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java:
##########
@@ -333,13 +333,15 @@ private ServerLauncher(final Builder builder) {
controlHandler = new ControlNotificationHandler() {
@Override
public void handleStop() {
+ logger.info("Handling ControllableProcess stop request.");
Review Comment:
Changed to debug.
--
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
[GitHub] [geode] Bill commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
Bill commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r879960279
##########
geode-junit/src/main/java/org/apache/geode/test/version/TestVersion.java:
##########
@@ -17,18 +17,24 @@
import java.io.Serializable;
import java.util.Objects;
+@SuppressWarnings("serial")
public class TestVersion implements Comparable<TestVersion>, Serializable {
- public static final TestVersion CURRENT_VERSION = new TestVersion(VersionManager.CURRENT_VERSION);
+
+ static final TestVersion CURRENT_VERSION = new TestVersion(VersionManager.CURRENT_VERSION);
private final int major;
private final int minor;
private final int release;
+ public static TestVersion current() {
+ return CURRENT_VERSION;
+ }
+
Review Comment:
It looks like this method was added to provide a little more encapsulation than exposing `CURRENT_VERSION` did. Only `VmConfiguration` continues to reference `CURRENT_VERSION`: I recommend making that class go through the method so `CURRENT_VERSION` can become `privsate`.
--
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
[GitHub] [geode] kirklund commented on pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on PR #7571:
URL: https://github.com/apache/geode/pull/7571#issuecomment-1135246054
ClusterCommunicationsDUnitTest failed one run of upgrade-test-openjdk11. I've searched Jira and now [I'm running that test through stress-new-test in another PR](https://github.com/apache/geode/pull/7719). I'm planning to file a ticket once I confirm it's flaky and unrelated to GfshRule changes.
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r880816466
##########
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java:
##########
@@ -333,13 +333,15 @@ private ServerLauncher(final Builder builder) {
controlHandler = new ControlNotificationHandler() {
@Override
public void handleStop() {
+ logger.debug("Handling ControllableProcess stop request.");
Review Comment:
Removed both statements
--
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
[GitHub] [geode] demery-pivotal commented on a diff in pull request #7571: [DRAFT] Add orphan prevention to GfshRule
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r869682714
##########
geode-junit/src/main/java/org/apache/geode/test/junit/rules/gfsh/GfshExecutor.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.test.junit.rules.gfsh;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeoutException;
+
+public interface GfshExecutor {
Review Comment:
Would it be reasonable to extract the common bits from `GfshRule` and `GfshContext` into this interface as default implementations? (This may not be worth the effort if we intend to try the rule/extension injection technique.)
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r878611092
##########
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java:
##########
@@ -333,13 +333,15 @@ private ServerLauncher(final Builder builder) {
controlHandler = new ControlNotificationHandler() {
@Override
public void handleStop() {
+ logger.info("Handling ControllableProcess stop request.");
Review Comment:
Are these log statements valuable? Should they be deleted?
--
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
[GitHub] [geode] kirklund commented on pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on PR #7571:
URL: https://github.com/apache/geode/pull/7571#issuecomment-1133486114
UPDATE: all checks have completed GREEN. Only LGTM Java is still in progress (it seems to be problematic lately)
--
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
[GitHub] [geode] kirklund merged pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund merged PR #7571:
URL: https://github.com/apache/geode/pull/7571
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7571:
URL: https://github.com/apache/geode/pull/7571#discussion_r880817043
##########
geode-junit/src/main/java/org/apache/geode/test/version/TestVersion.java:
##########
@@ -17,18 +17,24 @@
import java.io.Serializable;
import java.util.Objects;
+@SuppressWarnings("serial")
public class TestVersion implements Comparable<TestVersion>, Serializable {
- public static final TestVersion CURRENT_VERSION = new TestVersion(VersionManager.CURRENT_VERSION);
+
+ static final TestVersion CURRENT_VERSION = new TestVersion(VersionManager.CURRENT_VERSION);
private final int major;
private final int minor;
private final int release;
+ public static TestVersion current() {
+ return CURRENT_VERSION;
+ }
+
Review Comment:
Changed CURRENT_VERSION to private and VmConfiguration now uses current().
--
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
[GitHub] [geode] kirklund commented on pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
Posted by GitBox <gi...@apache.org>.
kirklund commented on PR #7571:
URL: https://github.com/apache/geode/pull/7571#issuecomment-1135072913
The latest precheckin run hit a flaky failure [GEODE-10090](https://issues.apache.org/jira/browse/GEODE-10090).
The previous precheckin was GREEN.
--
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