[GitHub] [geode] agingade commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService

[GitBox <gi...@apache.org>]

agingade commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600039453



##########
File path: buildSrc/src/main/java/com/pedjak/gradle/plugins/dockerizedtest/DockerizedExecHandle.java
##########
@@ -326,7 +326,7 @@ public void abort() {
       }
       if (!stateIn(ExecHandleState.STARTED, ExecHandleState.DETACHED)) {
         throw new IllegalStateException(
-            format("Cannot abort process '%s' because it is not in started or detached state",
+            format("Cannot abort process '%s' because it is not in started or detached state. It is currently in state: '%s'",state,

Review comment:
       The last two arguments needs to be swapped/reversed?

##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
##########
@@ -0,0 +1,261 @@
+/*
+ * 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.modules;
+
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import org.apache.geode.test.compiler.JarBuilder;
+import org.apache.geode.test.junit.rules.gfsh.GfshRule;
+import org.apache.geode.test.junit.rules.gfsh.GfshScript;
+
+public class DeployJarAcceptanceTest {
+
+  @ClassRule
+  public static GfshRule gfshRule = new GfshRule();
+
+  @ClassRule
+  public static TemporaryFolder stagingTempDir = new TemporaryFolder();
+
+  private static File jarFile;
+  private static File jarFileV2;
+  private static File anotherJarFile;
+
+  @BeforeClass
+  public static void setup() throws IOException {
+    File stagingDir = stagingTempDir.newFolder("staging");
+    jarFile = new File(stagingDir, "myJar-1.0.jar");
+    jarFileV2 = new File(stagingDir, "myJar-2.0.jar");
+    anotherJarFile = new File(stagingDir, "anotherJar-1.0.jar");
+    JarBuilder jarBuilder = new JarBuilder();
+    jarBuilder.buildJarFromClassNames(jarFile, "SomeClass");
+    jarBuilder.buildJarFromClassNames(jarFileV2, "SomeClass", "SomeClassVersionTwo");
+    jarBuilder.buildJarFromClassNames(anotherJarFile, "SomeOtherClass");
+
+    GfshScript
+        .of("start locator --name=locator", "configure pdx --read-serialized=true",
+            "start server --name=server --locators=localhost[10334]")
+        .execute(gfshRule);
+  }
+
+  @After
+  public void teardown() {
+    System.out.println(GfshScript.of(getLocatorGFSHConnectionString(), "undeploy")
+        .execute(gfshRule).getOutputText());
+  }
+
+  private String getLocatorGFSHConnectionString() {
+    return "connect --locator=localhost[10334]";
+  }
+
+  @Test
+  public void testDeployJar() throws IOException {
+    GfshScript.of(getLocatorGFSHConnectionString(),
+        "deploy --jar=" + jarFile.getCanonicalPath()).execute(gfshRule);
+
+    assertThat(GfshScript.of(getLocatorGFSHConnectionString(), "list deployed")
+        .execute(gfshRule).getOutputText()).contains(jarFile.getName()).contains("JAR Location");
+  }
+
+  @Test
+  public void testDeployJarWithDeploymentName() throws IOException {
+    GfshScript.of(getLocatorGFSHConnectionString(),
+        "deploy --name=myDeployment --jar=" + jarFile.getCanonicalPath()).execute(gfshRule);
+
+    assertThat(GfshScript.of(getLocatorGFSHConnectionString(), "list deployed")
+        .execute(gfshRule).getOutputText()).contains("myDeployment").contains("JAR Location");
+  }
+
+  @Test
+  public void testUndeployJar() throws IOException {
+    GfshScript.of(getLocatorGFSHConnectionString(),
+        "deploy --jar=" + jarFile.getCanonicalPath()).execute(gfshRule);
+
+    assertThat(
+        GfshScript.of(getLocatorGFSHConnectionString(),
+            "undeploy --jar=" + jarFile.getName())
+            .execute(gfshRule).getOutputText()).contains(jarFile.getName())
+                .contains("Un-Deployed From JAR Location");
+
+    assertThat(GfshScript.of(getLocatorGFSHConnectionString(), "list deployed")
+        .execute(gfshRule).getOutputText()).doesNotContain(jarFile.getName());
+  }
+
+  @Test
+  public void testUndeployWithNothingDeployed() {
+    assertThat(
+        GfshScript.of(getLocatorGFSHConnectionString(),
+            "undeploy --jar=" + jarFile.getName())
+            .execute(gfshRule).getOutputText()).contains(jarFile.getName() + " not deployed");
+  }
+
+  @Test
+  public void testRedeployNewJar() throws IOException {
+    GfshScript.of(getLocatorGFSHConnectionString(),
+        "deploy --jar=" + jarFile.getCanonicalPath()).execute(gfshRule);
+
+    assertThat(
+        GfshScript.of(getLocatorGFSHConnectionString(),
+            "undeploy --jar=" + jarFile.getName())
+            .execute(gfshRule).getOutputText()).contains(jarFile.getName())
+                .contains("Un-Deployed From JAR Location");
+
+    assertThat(GfshScript.of(getLocatorGFSHConnectionString(), "list deployed")
+        .execute(gfshRule).getOutputText()).doesNotContain(jarFile.getName());
+
+    GfshScript
+        .of(getLocatorGFSHConnectionString(),
+            "deploy --jar=" + anotherJarFile.getCanonicalPath())
+        .execute(gfshRule);
+    assertThat(GfshScript.of(getLocatorGFSHConnectionString(), "list deployed")
+        .execute(gfshRule).getOutputText()).contains(anotherJarFile.getName());
+  }
+
+  @Test
+  public void testUpdateJar() throws IOException {
+    GfshScript.of(getLocatorGFSHConnectionString(),
+        "deploy --jar=" + jarFile.getCanonicalPath()).execute(gfshRule);
+
+    GfshScript.of(getLocatorGFSHConnectionString(),

Review comment:
       Do we need to test where jar with same name is deployed? And new class/functionality could be invoked successfully.
   usecase: jar getting updated with additional class.
   
   Ignore this if its already there in other tests...

##########
File path: geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/DeploymentManagementController.java
##########
@@ -63,7 +67,7 @@
       @RequestParam(required = false) String group) {
     Deployment deployment = new Deployment();
     if (StringUtils.isNotBlank(id)) {
-      deployment.setFileName(id);
+      deployment.setDeploymentName(id);

Review comment:
       I don't know much about this area...but thought of asking this; with these changes does an older rest client can work with newer cluster...

##########
File path: geode-wan/src/main/java/org/apache/geode/internal/WANDistributedSystemService.java
##########
@@ -20,6 +20,7 @@
 
 import org.apache.geode.distributed.internal.DistributedSystemService;
 import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.classloader.ClassPathLoader;

Review comment:
       Is this addition for build purpose...Not seeing any changes in the class file.




-- 
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