[GitHub] [geode] kirklund commented on a diff in pull request #7571: GEODE-10327: Overhaul GfshRule to kill processes and save artifacts for failures
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, 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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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, 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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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 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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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 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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@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
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 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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
