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 2021/03/16 02:52:15 UTC
[GitHub] [geode] kohlmu-pivotal opened a new pull request #6144: GEODE-8905: Introduce JarDeploymentService (#5989)
kohlmu-pivotal opened a new pull request #6144:
URL: https://github.com/apache/geode/pull/6144
* GEODE-8905: Introduce JarDeploymentService.
Unified deploy functionality between REST and GFSH
Pulled deploy jar functionality into its own module
Renamed internal *Util classes to *Utils
Co-authored-by: Udo Kohlmeyer <uk...@pivotal.io>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-804548883
This pull request **introduces 1 alert** and **fixes 1** when merging 16c7e34318101d619e438ce8b4707d19fba996e2 into 55a6e065960694bbe7856cbea186c55aac6c7867 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-9ba4fac7414463c02f101435a5086cb8fde0eb2e)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r599088721
##########
File path: geode-core/build.gradle
##########
@@ -233,6 +233,8 @@ dependencies {
exclude group: '*'
}
+ runtimeOnly(project(':geode-deployment-legacy'))
Review comment:
The deployment code has been decoupled and replaced with a JarDeploymentService interface. The runtime dependency is it ensure that we have a runtime dependency on this module, but not compile time.
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600069866
##########
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:
This is because the ClassPathLoader was moved
https://github.com/apache/geode/pull/6144/files#diff-4bd2f197c6e3442a0906cea9b03ecd0b0bdc5d74070d2e59c2e6c45e21f5ec47
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-808624503
This pull request **introduces 1 alert** and **fixes 1** when merging a8bd4fdc12fd4b1d8bda53860626c9d3bf128c10 into 384196a15d7ff5807c4c1a5b141f196329a7029a - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-8f5774d6a74b5df7484646e8f68a442bf3153d8c)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-804399059
This pull request **introduces 1 alert** and **fixes 1** when merging 4dbcf1fa61726224ddd53253c07b5e3987b35086 into 84b2ff300b2e98e6600da26863be49bf0182c33e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-580b8a9bcb47f26923735bd1de082035ef72902b)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600068741
##########
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:
https://github.com/apache/geode/blob/16c7e34318101d619e438ce8b4707d19fba996e2/geode-management/src/main/java/org/apache/geode/management/configuration/Deployment.java#L122-L129
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600151178
##########
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:
@agingade good catch. We tried to make that backward compatibility is met.
I've added tests to support that now in https://github.com/apache/geode/pull/6144/commits/9d1af5af3d71b9491fcbf786bc8313c2137504e4#diff-824a5037676a6a6e678516d51503f74d2493e960080ada8b66911571836e722e
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600151178
##########
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:
@agingade good catch. We tried to make sure that backward compatibility is met.
I've added tests to support that now in https://github.com/apache/geode/pull/6144/commits/9d1af5af3d71b9491fcbf786bc8313c2137504e4#diff-824a5037676a6a6e678516d51503f74d2493e960080ada8b66911571836e722e
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600065820
##########
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:
Yes, you are correct... Should have caught that. Thank you
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600067178
##########
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:
Yes.. DeploymentName can be derived from the Filename if none is provided. This is to make sure that the gfsh command is backwards compatible as well.
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600066611
##########
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:
I'll make sure that there is a test covering that case
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600045072
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/InternalConfigurationPersistenceService.java
##########
@@ -373,22 +384,28 @@ public boolean removeJars(String[] jarNames, String[] groups) {
break;
}
- for (String jarRemoved : jarNames) {
- File jar = getPathToJarOnThisLocator(group, jarRemoved).toFile();
+ logger.debug("Configuration before removing deployment: " + configuration);
Review comment:
Yes, we found that without debug logging, it was hard debugging it whilst running.
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService (#5989)
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-802441042
This pull request **introduces 1 alert** and **fixes 1** when merging f2a8bd073058d2878a4446c8f852f6bcf54a7211 into 0870d5dc2fd769d51f63ec7e0d64c9c8cb4d2c8f - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-2a21b01b33d54ae4795a3687ac43ef8698f02931)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService (#5989)
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-801443386
This pull request **introduces 1 alert** and **fixes 1** when merging a449622c274bc07f524997328f1738c509842753 into dfc16562a1965d4d0bc32756bdf2a9664b4127c2 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-ffd81a07bdc86436de252dd88daedc9e5f46480f)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
----------------------------------------------------------------
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600067178
##########
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:
Yes.. DeploymentName can be derived from the Filename if none is provided. This is to make sure that the gfsh command is backwards compatible as well.
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService (#5989)
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-800717315
This pull request **introduces 1 alert** and **fixes 1** when merging 783205624a691fd17c3f2d0fbcd7f33fd2c45e33 into 1fb1dc186cb2aeaa97606a8fe9488128bac2f808 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-c4dc427f6b434d71c11223bb711e12c2b4f8c433)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
----------------------------------------------------------------
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r599083354
##########
File path: geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
##########
@@ -117,16 +117,17 @@ public void listById() throws Exception {
assertManagementListResult(list).isSuccessful();
resultAssert.hasConfigurations().extracting(Deployment::getFileName)
.containsExactlyInAnyOrder("lib.jar", "lib.jar");
Review comment:
I agree with you. The order should matter and `containsExactlyInAnyOrder` seems to disregard this. Given that this file has only cosmetic changes (who knows why the formatting changed now, given that spotless is always applied) and no code changes have been made in this file, I would hope the review would be as such.
I would love to fix all problems in all files that I touch, but in all honesty, I don't have time to fix them 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600068741
##########
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:
https://github.com/apache/geode/blob/16c7e34318101d619e438ce8b4707d19fba996e2/geode-management/src/main/java/org/apache/geode/management/configuration/Deployment.java#L122-L129
--
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] rhoughton-pivot commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r599071157
##########
File path: geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
##########
@@ -117,16 +117,17 @@ public void listById() throws Exception {
assertManagementListResult(list).isSuccessful();
resultAssert.hasConfigurations().extracting(Deployment::getFileName)
.containsExactlyInAnyOrder("lib.jar", "lib.jar");
Review comment:
Doesn't the classpath order matter? This feels like a bad way to compare.
##########
File path: geode-core/build.gradle
##########
@@ -233,6 +233,8 @@ dependencies {
exclude group: '*'
}
+ runtimeOnly(project(':geode-deployment-legacy'))
Review comment:
After looking at the contents of `geode-core/build.gradle` I am not sure what this line is buying us on the classpath or POM 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
[GitHub] [geode] kohlmu-pivotal merged pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal merged pull request #6144:
URL: https://github.com/apache/geode/pull/6144
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService (#5989)
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-801524207
This pull request **introduces 1 alert** and **fixes 1** when merging 57be428dc51d995d56fd391e6fa38849c562b496 into 14e1cfca58f9efb2b9bde0e51e5d1671ff6e9745 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-c1ea95a914e7779bdab809e481bde591141fceb8)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
----------------------------------------------------------------
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService (#5989)
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-802367822
This pull request **introduces 1 alert** and **fixes 1** when merging 6df94e288d0078f67dd6897690ae44354a29432c into 0870d5dc2fd769d51f63ec7e0d64c9c8cb4d2c8f - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-807f921c5781799fbc0ff7838679c3def30ff978)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-808590224
This pull request **introduces 1 alert** and **fixes 1** when merging edcea7609fbcef1643117e52a9d80fff75f2dee1 into 55ee370ca436f10f5acd383867039ccc1055fd27 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-c21a7c7ed57c540ab07efb83df75596f90b7a276)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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] agingade commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by 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
[GitHub] [geode] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-805491678
This pull request **introduces 1 alert** and **fixes 1** when merging 9d1af5af3d71b9491fcbf786bc8313c2137504e4 into a5bd36f9fa787d3a71c6e6efafed5a7b0fe52d2b - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-761b6df429f71c80c1a91d98033d86637cba5dbd)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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] rhoughton-pivot commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r599086200
##########
File path: geode-core/build.gradle
##########
@@ -233,6 +233,8 @@ dependencies {
exclude group: '*'
}
+ runtimeOnly(project(':geode-deployment-legacy'))
Review comment:
I wasn't considering the actual class files from the new service.
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600069866
##########
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:
This is because the ClassPathLoader was moved from "org.apache.geode.internal" -> "org.apache.geode.internal.classloader"
--
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] Bill commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600036365
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/InternalConfigurationPersistenceService.java
##########
@@ -373,22 +384,28 @@ public boolean removeJars(String[] jarNames, String[] groups) {
break;
}
- for (String jarRemoved : jarNames) {
- File jar = getPathToJarOnThisLocator(group, jarRemoved).toFile();
+ logger.debug("Configuration before removing deployment: " + configuration);
+ Configuration configurationCopy = new Configuration(configuration);
+
+ for (Entry<String, String> deploymentToRemove : deploymentsToRemove.entrySet()) {
+ File jar = getPathToJarOnThisLocator(group, deploymentToRemove.getValue()).toFile();
if (jar.exists()) {
try {
FileUtils.forceDelete(jar);
+ logger.debug("Successfully deleted: " + jar.getName());
+ configurationCopy
+ .removeDeployments(Collections.singleton(deploymentToRemove.getKey()));
Review comment:
Just looking at the diff here it appears that removal was happening only once after the loop but that now it happens (potentially) each time through the loop. Not a problem but I just wanted to confirm that's what you wanted.
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/InternalConfigurationPersistenceServiceDeploymentTest.java
##########
@@ -112,10 +112,7 @@ public void addPlainJarToThisLocator() throws Exception {
}
private Deployment createDeployment(String jarFileName, String deployedby, String deployedtime) {
- Deployment result = new Deployment();
- result.setFileName(jarFileName);
- result.setDeployedBy(deployedby);
- result.setDeployedTime(deployedtime);
+ Deployment result = new Deployment(jarFileName, deployedby, deployedtime);
Review comment:
better!
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/InternalConfigurationPersistenceService.java
##########
@@ -373,22 +384,28 @@ public boolean removeJars(String[] jarNames, String[] groups) {
break;
}
- for (String jarRemoved : jarNames) {
- File jar = getPathToJarOnThisLocator(group, jarRemoved).toFile();
+ logger.debug("Configuration before removing deployment: " + configuration);
Review comment:
Lots of debug logging here. Do you really want to keep all that?
--
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] kohlmu-pivotal commented on a change in pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #6144:
URL: https://github.com/apache/geode/pull/6144#discussion_r600044706
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/InternalConfigurationPersistenceService.java
##########
@@ -373,22 +384,28 @@ public boolean removeJars(String[] jarNames, String[] groups) {
break;
}
- for (String jarRemoved : jarNames) {
- File jar = getPathToJarOnThisLocator(group, jarRemoved).toFile();
+ logger.debug("Configuration before removing deployment: " + configuration);
+ Configuration configurationCopy = new Configuration(configuration);
+
+ for (Entry<String, String> deploymentToRemove : deploymentsToRemove.entrySet()) {
+ File jar = getPathToJarOnThisLocator(group, deploymentToRemove.getValue()).toFile();
if (jar.exists()) {
try {
FileUtils.forceDelete(jar);
+ logger.debug("Successfully deleted: " + jar.getName());
+ configurationCopy
+ .removeDeployments(Collections.singleton(deploymentToRemove.getKey()));
Review comment:
Yes, this was intentional.
--
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService (#5989)
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-799924564
This pull request **introduces 1 alert** and **fixes 1** when merging 9637ddd1490b55c85c7179a72748019bb5d53981 into 5fe934e464b3a41a3dd99b9d019cb4a0de61aae6 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-f6d8227fcdc466390d85b77e5dbda53649c01cfa)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
----------------------------------------------------------------
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] lgtm-com[bot] commented on pull request #6144: GEODE-8905: Introduce JarDeploymentService
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6144:
URL: https://github.com/apache/geode/pull/6144#issuecomment-805436090
This pull request **introduces 1 alert** and **fixes 1** when merging 4c760f0b1b7d73f330d146eba6c8546ff0a96d84 into a5bd36f9fa787d3a71c6e6efafed5a7b0fe52d2b - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-5522596bc96df0877a26a6b15a1745b2dbd77a2d)
**new alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
**fixed alerts:**
* 1 for Use of a broken or risky cryptographic algorithm
--
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