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 2020/06/24 21:06:14 UTC
[GitHub] [geode] jinmeiliao opened a new pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
jinmeiliao opened a new pull request #5299:
URL: https://github.com/apache/geode/pull/5299
* improve some backward compatibility test to cover more versions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5299:
URL: https://github.com/apache/geode/pull/5299#discussion_r445226334
##########
File path: geode-assembly/src/upgradeTest/java/org/apache/geode/management/DeploymentManagementUpgradeTest.java
##########
@@ -55,16 +84,16 @@ public static void beforeClass() throws Exception {
@Test
public void newLocatorCanReadOldConfigurationData() throws IOException {
- File workingDir = tempFolder.newFolder();
int[] ports = AvailablePortHelper.getRandomAvailableTCPPorts(3);
- oldGfsh.execute("start locator --name=test --port=" + ports[0] + " --http-service-port="
- + ports[1] + " --dir=" + workingDir.getAbsolutePath() + " --J=-Dgemfire.jmx-manager-port="
- + ports[2],
- "deploy --jar=" + clusterJar.getAbsolutePath(),
- "shutdown --include-locators");
+ GfshExecution execute =
+ GfshScript.of(startLocatorCommand("test", ports[0], ports[2], ports[1], 0))
Review comment:
For code readability, it is better to have some meaningful names for the ports. Before the code change, it is easy to identify the purpose of a port with the gfsh option in the code. Now it is not that straight forward to tell ports[0] is locator port. port[2] is JMX port and ports[1] is HTTP port, unless you see to the definition of `startLocatorCommand`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5299:
URL: https://github.com/apache/geode/pull/5299#discussion_r445693168
##########
File path: geode-junit/src/main/java/org/apache/geode/test/junit/rules/gfsh/GfshRule.java
##########
@@ -199,4 +220,23 @@ private void stopMembers(GfshExecution gfshExecution) {
}
execute(GfshScript.of(stopMemberScripts).withName("Stop-Members"));
}
+
+ public static String startServerCommand(String name, int port, int connectedLocatorPort) {
Review comment:
There are actually a couple existing classes like this that we could move to geode-junit and make any necessary changes:
```
geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorCommand.java
geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerCommand.java
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5299:
URL: https://github.com/apache/geode/pull/5299#discussion_r445691690
##########
File path: geode-junit/src/main/java/org/apache/geode/test/junit/rules/gfsh/GfshRule.java
##########
@@ -199,4 +220,23 @@ private void stopMembers(GfshExecution gfshExecution) {
}
execute(GfshScript.of(stopMemberScripts).withName("Stop-Members"));
}
+
+ public static String startServerCommand(String name, int port, int connectedLocatorPort) {
Review comment:
These new static methods don't really fit well in GfshRule. I think you should probably move them to two new classes (probably not a rule) named something like ServerCommandBuilder and LocatorCommandBuilder, and explode the parameters to each have their own setter type method:
```
String startServerCommand = new ServerCommandBuilder()
.withPort(port)
.withLocator(locatorPort)
.create(name);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao merged pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
Posted by GitBox <gi...@apache.org>.
jinmeiliao merged pull request #5299:
URL: https://github.com/apache/geode/pull/5299
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5299:
URL: https://github.com/apache/geode/pull/5299#discussion_r445695124
##########
File path: geode-junit/src/main/java/org/apache/geode/test/junit/rules/gfsh/GfshRule.java
##########
@@ -199,4 +220,23 @@ private void stopMembers(GfshExecution gfshExecution) {
}
execute(GfshScript.of(stopMemberScripts).withName("Stop-Members"));
}
+
+ public static String startServerCommand(String name, int port, int connectedLocatorPort) {
Review comment:
We could commit what you have ready and then make further changes. I'll go ahead and approve and let you decide.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5299:
URL: https://github.com/apache/geode/pull/5299#discussion_r445705089
##########
File path: geode-junit/src/main/java/org/apache/geode/test/junit/rules/gfsh/GfshRule.java
##########
@@ -199,4 +220,23 @@ private void stopMembers(GfshExecution gfshExecution) {
}
execute(GfshScript.of(stopMemberScripts).withName("Stop-Members"));
}
+
+ public static String startServerCommand(String name, int port, int connectedLocatorPort) {
Review comment:
Thanks! I will merge this now and we can make improvements on it when we get to consolidating all these methods into one place.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5299: GEODE-8200: enhance GfshRule to specify a working dir
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5299:
URL: https://github.com/apache/geode/pull/5299#discussion_r445693168
##########
File path: geode-junit/src/main/java/org/apache/geode/test/junit/rules/gfsh/GfshRule.java
##########
@@ -199,4 +220,23 @@ private void stopMembers(GfshExecution gfshExecution) {
}
execute(GfshScript.of(stopMemberScripts).withName("Stop-Members"));
}
+
+ public static String startServerCommand(String name, int port, int connectedLocatorPort) {
Review comment:
There are actually a couple existing classes like this that we could move to geode-dunit and make any necessary changes:
```
geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorCommand.java
geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerCommand.java
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org