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/02/02 01:10:35 UTC

[GitHub] [geode] yozaner1324 opened a new pull request #5989: Geode 8905

yozaner1324 opened a new pull request #5989:
URL: https://github.com/apache/geode/pull/5989


   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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-784757920


   This pull request **introduces 1 alert** and **fixes 1** when merging eaa0070c4d9525f40946e23236faf86176a30013 into 3a21c2852746f19755ac302f584ca5b8908eae2e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-425506d6ee44433062ec391a8bf7f937d3fb96f8)
   
   **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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r581358382



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
##########
@@ -94,26 +92,16 @@ public void deployJarsReceivedFromClusterConfiguration(ConfigurationResponse res
             .flatMap(Set::stream)
             .collect(Collectors.toList());
 
-    if (jarFileNames != null && !jarFileNames.isEmpty()) {
+    if (!jarFileNames.isEmpty()) {
       logger.info("Got response with jars: {}", jarFileNames.stream().collect(joining(",")));
-      JarDeployer jarDeployer = ClassPathLoader.getLatest().getJarDeployer();
-      jarDeployer.suspendAll();
-      try {
-        Set<File> stagedJarFiles =
-            getJarsFromLocator(response.getMember(), response.getJarNames());
-
-        for (File stagedJarFile : stagedJarFiles) {
-          logger.info("Removing old versions of {} in cluster configuration.",

Review comment:
       Put the logging back

##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
##########
@@ -129,15 +133,17 @@ public void deployNewVersionOfFunctionOverOldVersion() throws Exception {
     GemFireCache gemFireCache = server.getCache();
     DistributedSystem distributedSystem = gemFireCache.getDistributedSystem();
 
-    ClassPathLoader.getLatest().getJarDeployer().deploy(jarVersion1);
+    JarDeploymentServiceFactory.getJarDeploymentServiceInstance()
+        .deploy(createDeploymentFromJar(jarVersion1));
 
     assertThatClassCanBeLoaded("jddunit.function.MyFunction");
     Execution execution = FunctionService.onMember(distributedSystem.getDistributedMember());
 
     List<String> result = (List<String>) execution.execute("MyFunction").getResult();
     assertThat(result.get(0)).isEqualTo("Version1");
 
-    ClassPathLoader.getLatest().getJarDeployer().deploy(jarVersion2);
+    JarDeploymentServiceFactory.getJarDeploymentServiceInstance()

Review comment:
       Moved to 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] rhoughton-pivot commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577341266



##########
File path: buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
##########
@@ -172,17 +179,12 @@ class DependencyConstraints implements Plugin<Project> {
       }
     }
 
-    dependencySet(group: 'com.fasterxml.jackson.core', version: '2.12.1') {
+    dependencySet(group: 'com.fasterxml.jackson.core', version: get('jackson.version')) {
       entry('jackson-annotations')
       entry('jackson-core')
       entry('jackson-databind')
     }
 
-    dependencySet(group: 'com.fasterxml.jackson.datatype', version: '2.12.1') {
-      entry('jackson-datatype-joda')

Review comment:
       Is there something about the other form of this declaration that we like better?
   

##########
File path: buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
##########
@@ -87,6 +91,8 @@ class DependencyConstraints implements Plugin<Project> {
         api(group: 'cglib', name: 'cglib', version: get('cglib.version'))
         api(group: 'com.arakelian', name: 'java-jq', version: '1.1.0')
         api(group: 'com.carrotsearch.randomizedtesting', name: 'randomizedtesting-runner', version: '2.7.8')
+        api(group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-joda', version: get('jackson.version'))

Review comment:
       These had been declared as part of a `dependencySet` on about line 181. Why did they move?

##########
File path: extensions/geode-modules-session/build.gradle
##########
@@ -29,6 +29,7 @@ dependencies {
     exclude module: 'geode-modules'
   }
   compile(project(':geode-core'))
+  compile(project(':geode-common'))

Review comment:
       Lets prefer moving to `implementation` for all new dependencies we add, instead of propagating deprecated configuration types 

##########
File path: geode-core/build.gradle
##########
@@ -311,7 +313,7 @@ dependencies {
     implementation('com.healthmarketscience.rmiio:rmiio')
 
     //Geode-common has annotations and other pieces used geode-core
-    api(project(':geode-common'))
+    implementation(project(':geode-common'))

Review comment:
       This needs to be `api`. `geode-common` declares `Identifiable`, which is implemented by `CacheElement` in `geode-core`. There are other examples.

##########
File path: geode-jboss-extensions/build.gradle
##########
@@ -0,0 +1,28 @@
+import org.apache.geode.gradle.plugins.DependencyConstraints
+
+/*
+ * 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.
+ */
+
+apply from: "${rootDir}/${scriptDir}/standard-subproject-configuration.gradle"
+
+apply from: "${project.projectDir}/../gradle/publish-java.gradle"
+
+
+dependencies {
+    implementation(platform(project(':boms:geode-all-bom')))
+    compileOnly('org.jboss.modules:jboss-modules:' + DependencyConstraints.get("jboss-modules.version"))

Review comment:
       Changing the platform dependency from `implementation` to `compileOnly` will let you remove the `DependencyConstraints.get()` from line 27.




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal merged pull request #5989:
URL: https://github.com/apache/geode/pull/5989


   


----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577896083



##########
File path: buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
##########
@@ -87,6 +91,8 @@ class DependencyConstraints implements Plugin<Project> {
         api(group: 'cglib', name: 'cglib', version: get('cglib.version'))
         api(group: 'com.arakelian', name: 'java-jq', version: '1.1.0')
         api(group: 'com.carrotsearch.randomizedtesting', name: 'randomizedtesting-runner', version: '2.7.8')
+        api(group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-joda', version: get('jackson.version'))

Review comment:
       Reverted to using dependencySet instead




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-785526536


   This pull request **introduces 1 alert** and **fixes 1** when merging f7670c2475e3053558ee4f4c45bf6e3734eb5a72 into e08cad4d7198f9bd82614ca9b7e631df36b38757 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-45ab3c60875ff003c72ea8d1508b77f78e5fc429)
   
   **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] kirklund commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r581393444



##########
File path: geode-dunit/src/main/java/org/apache/geode/management/internal/configuration/ClusterConfig.java
##########
@@ -57,6 +59,27 @@ public ClusterConfig(ConfigGroup... configGroups) {
     Collections.addAll(this.groups, configGroups);
   }
 
+  private static Set<String> toSetIgnoringHiddenFiles(String[] array) {
+    if (array == null) {
+      return new HashSet<>();

Review comment:
       If callers cannot modify the returned Set, then you might want to return Collections.emptySet() instead.

##########
File path: geode-deployment-legacy/src/main/java/org/apache/geode/deployment/impl/legacy/LegacyJarDeploymentService.java
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.deployment.impl.legacy;

Review comment:
       Should `org.apache.geode.deployment.impl.legacy` be `org.apache.geode.deployment.internal.legacy` instead?

##########
File path: geode-deployment-legacy/src/main/java/org/apache/geode/deployment/impl/legacy/LegacyJarDeploymentService.java
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.deployment.impl.legacy;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.time.Instant;
+import java.util.Collections;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+
+import org.apache.logging.log4j.Logger;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.deployment.JarDeploymentService;
+import org.apache.geode.internal.DeployedJar;
+import org.apache.geode.internal.JarDeployer;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.management.configuration.Deployment;
+import org.apache.geode.services.result.ServiceResult;
+import org.apache.geode.services.result.impl.Failure;
+import org.apache.geode.services.result.impl.Success;
+
+/**
+ * Implementation of {@link JarDeploymentService} that encpsulates the standard method of deploying
+ * jars by delegating to the {@link JarDeployer}.
+ *
+ * @since Geode 1.15
+ */
+@Experimental
+public class LegacyJarDeploymentService implements JarDeploymentService {
+
+  private static final Logger logger = LogService.getLogger();
+
+  private final Map<String, Deployment> deployments;
+  private JarDeployer jarDeployer;

Review comment:
       Just scanning over this class, it looks like `jarDeployer` can probably be `final`.

##########
File path: geode-core/src/main/java/org/apache/geode/services/result/ServiceResult.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.services.result;
+
+import java.util.function.Consumer;
+import java.util.function.Function;
+
+/**
+ * The {@link ServiceResult} type is an attempt at the function construct of
+ * <a href="https://www.vavr.io/vavr-docs/#_either">Either</a>. In this implementation a
+ * {@link ServiceResult}
+ * can define either success or failure (error) using the same type.
+ *
+ * @param <SuccessType> the return type for a successful operation.
+ *
+ * @since 1.14.0
+ */
+public interface ServiceResult<SuccessType> extends Result<SuccessType, String> {
+  /**
+   * {@inheritDoc}

Review comment:
       I think you should just delete any uses of `@inheritDoc`. You can then delete the overridden method definitions since they're already in the super interface.




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-784628827


   This pull request **introduces 1 alert** and **fixes 1** when merging 45a2576e6a84b7c104679c72e1f4735994242a1f into 3a21c2852746f19755ac302f584ca5b8908eae2e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-256b76595ba60fa215acf96e38130b8e852042e3)
   
   **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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r578046017



##########
File path: geode-core/build.gradle
##########
@@ -233,6 +233,8 @@ dependencies {
         exclude group: '*'
     }
 
+    runtimeOnly(project(':geode-deployment-legacy'))

Review comment:
       I think if you run `./gradlew geode-deployment-legacy:dependencies` you will see geode-core  is transitive on the compile classpath.




----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577895620



##########
File path: geode-core/build.gradle
##########
@@ -311,7 +313,7 @@ dependencies {
     implementation('com.healthmarketscience.rmiio:rmiio')
 
     //Geode-common has annotations and other pieces used geode-core
-    api(project(':geode-common'))
+    implementation(project(':geode-common'))

Review comment:
       Changed back to api.




----------------------------------------------------------------
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] bschuchardt commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r579469077



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
##########
@@ -129,15 +133,17 @@ public void deployNewVersionOfFunctionOverOldVersion() throws Exception {
     GemFireCache gemFireCache = server.getCache();
     DistributedSystem distributedSystem = gemFireCache.getDistributedSystem();
 
-    ClassPathLoader.getLatest().getJarDeployer().deploy(jarVersion1);
+    JarDeploymentServiceFactory.getJarDeploymentServiceInstance()
+        .deploy(createDeploymentFromJar(jarVersion1));
 
     assertThatClassCanBeLoaded("jddunit.function.MyFunction");
     Execution execution = FunctionService.onMember(distributedSystem.getDistributedMember());
 
     List<String> result = (List<String>) execution.execute("MyFunction").getResult();
     assertThat(result.get(0)).isEqualTo("Version1");
 
-    ClassPathLoader.getLatest().getJarDeployer().deploy(jarVersion2);
+    JarDeploymentServiceFactory.getJarDeploymentServiceInstance()

Review comment:
       We would like to move everything in org.apache.geode.internal into more specific packages that are pertinent to their function.  Please move ClassPathLoader and associated tests to a different package as part of this PR.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
##########
@@ -94,26 +92,16 @@ public void deployJarsReceivedFromClusterConfiguration(ConfigurationResponse res
             .flatMap(Set::stream)
             .collect(Collectors.toList());
 
-    if (jarFileNames != null && !jarFileNames.isEmpty()) {
+    if (!jarFileNames.isEmpty()) {
       logger.info("Got response with jars: {}", jarFileNames.stream().collect(joining(",")));
-      JarDeployer jarDeployer = ClassPathLoader.getLatest().getJarDeployer();
-      jarDeployer.suspendAll();
-      try {
-        Set<File> stagedJarFiles =
-            getJarsFromLocator(response.getMember(), response.getJarNames());
-
-        for (File stagedJarFile : stagedJarFiles) {
-          logger.info("Removing old versions of {} in cluster configuration.",

Review comment:
       Some Info-level log statements have been lost here.  Why are they no longer needed?




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-785627405


   This pull request **introduces 1 alert** and **fixes 1** when merging 7b26631c17218f2a4c7b416d1a833c7033d06cc4 into e08cad4d7198f9bd82614ca9b7e631df36b38757 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-cfa5552c292aa17066d5edf29ff83155d32f7d3e)
   
   **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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r581447533



##########
File path: geode-deployment-legacy/src/main/java/org/apache/geode/deployment/impl/legacy/LegacyJarDeploymentService.java
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.deployment.impl.legacy;

Review comment:
       Yes, that change is coming.




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-786174094


   This pull request **introduces 1 alert** and **fixes 1** when merging 2a5af1fb034a176cc975789d21b1ad8116aff8b1 into e08cad4d7198f9bd82614ca9b7e631df36b38757 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-c8631af40cdd3e129db3951388abd3c5f9fb5168)
   
   **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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r581497401



##########
File path: geode-core/src/main/java/org/apache/geode/deployment/JarDeploymentService.java
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.deployment;
+
+import java.io.File;
+import java.util.List;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.api.ClusterManagementService;
+import org.apache.geode.management.configuration.Deployment;
+import org.apache.geode.services.result.ServiceResult;
+
+/**
+ * Implementations of {@link JarDeploymentService} will be responsible for deploying jars into Geode
+ * from both gfsh and the {@link ClusterManagementService}.
+ *
+ * @since Geode 1.15
+ */
+@Experimental
+public interface JarDeploymentService {

Review comment:
       It can be internal, we will move it.




----------------------------------------------------------------
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] onichols-pivotal commented on a change in pull request #5989: Geode 8905 - Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577217101



##########
File path: buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
##########
@@ -87,6 +92,8 @@ class DependencyConstraints implements Plugin<Project> {
         api(group: 'cglib', name: 'cglib', version: get('cglib.version'))
         api(group: 'com.arakelian', name: 'java-jq', version: '1.1.0')
         api(group: 'com.carrotsearch.randomizedtesting', name: 'randomizedtesting-runner', version: '2.7.8')
+        api(group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-joda', version: '2.9.8')
+        api(group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-jsr310', version: '2.11.4')

Review comment:
       the current version on develop of datatype-joda and datatype-jsr310 is and should remain 2.12.1, as set down below around line 189, so please delete these two lines




----------------------------------------------------------------
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 pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-780861750


   > I'm not sure why I was included as a reviewer here & have no comments on the changes. So as not to block the PR I'm approving it, but others need to give it a +1 before it's merged.
   
   @bschuchardt Have you used the "File Filter -> Your CODEOWNER Files" functionality? Donal shared a screen shot in Slack about it.


----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r578024927



##########
File path: geode-core/build.gradle
##########
@@ -233,6 +233,8 @@ dependencies {
         exclude group: '*'
     }
 
+    runtimeOnly(project(':geode-deployment-legacy'))

Review comment:
       geode-core only needs geode-deployment-legacy at runtime to load its implementation of JarDeploymentService; It doesn't need it at compile time. I tried removing the runtime dependency to see if it was coming from somewhere else, and it wasn't - things broke at runtime.




----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577845023



##########
File path: extensions/geode-modules-session/build.gradle
##########
@@ -29,6 +29,7 @@ dependencies {
     exclude module: 'geode-modules'
   }
   compile(project(':geode-core'))
+  compile(project(':geode-common'))

Review comment:
       Replaced compile with implementation.




----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577844712



##########
File path: buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
##########
@@ -172,17 +179,12 @@ class DependencyConstraints implements Plugin<Project> {
       }
     }
 
-    dependencySet(group: 'com.fasterxml.jackson.core', version: '2.12.1') {
+    dependencySet(group: 'com.fasterxml.jackson.core', version: get('jackson.version')) {
       entry('jackson-annotations')
       entry('jackson-core')
       entry('jackson-databind')
     }
 
-    dependencySet(group: 'com.fasterxml.jackson.datatype', version: '2.12.1') {
-      entry('jackson-datatype-joda')

Review comment:
       I reverted to using the dependencySet form.




----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r578028301



##########
File path: geode-deployment-legacy/build.gradle
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.
+ */
+
+apply from: "${project.projectDir}/../gradle/standard-subproject-configuration.gradle"
+
+apply from: "${project.projectDir}/../gradle/publish-java.gradle"
+apply from: "${project.projectDir}/../gradle/warnings.gradle"
+
+dependencies {
+    implementation(platform(project(':boms:geode-all-bom')))
+    compileOnly(project(':geode-core'))

Review comment:
       Changed it to implementation




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-786882526


   This pull request **introduces 1 alert** and **fixes 1** when merging 9fe3c2459888eee5423c938e6741d9fea15109c6 into 406c8b4db24cb2a9384b5ab9389c1aed3b0cf802 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-1674feed04860fb2bf332052c62dd5604eab702f)
   
   **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] yozaner1324 commented on a change in pull request #5989: Geode 8905 - Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577239449



##########
File path: buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
##########
@@ -87,6 +92,8 @@ class DependencyConstraints implements Plugin<Project> {
         api(group: 'cglib', name: 'cglib', version: get('cglib.version'))
         api(group: 'com.arakelian', name: 'java-jq', version: '1.1.0')
         api(group: 'com.carrotsearch.randomizedtesting', name: 'randomizedtesting-runner', version: '2.7.8')
+        api(group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-joda', version: '2.9.8')
+        api(group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-jsr310', version: '2.11.4')

Review comment:
       Fixed




----------------------------------------------------------------
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] removed a comment on pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] removed a comment on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-785430978






----------------------------------------------------------------
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] dschneider-pivotal commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r581493894



##########
File path: geode-core/src/main/java/org/apache/geode/deployment/JarDeploymentService.java
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.deployment;
+
+import java.io.File;
+import java.util.List;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.api.ClusterManagementService;
+import org.apache.geode.management.configuration.Deployment;
+import org.apache.geode.services.result.ServiceResult;
+
+/**
+ * Implementations of {@link JarDeploymentService} will be responsible for deploying jars into Geode
+ * from both gfsh and the {@link ClusterManagementService}.
+ *
+ * @since Geode 1.15
+ */
+@Experimental
+public interface JarDeploymentService {

Review comment:
       Why is this interface not in an internal package? It seems like it could be in the same package as JarDeploymentServiceFactory which is internal. Does some other non-internal api expose this interface? Would a geode user want to implement this interface? By making it internal you don't need to worry about marking it experimental because internal apis are free to change or be removed.




----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577845953



##########
File path: geode-jboss-extensions/build.gradle
##########
@@ -0,0 +1,28 @@
+import org.apache.geode.gradle.plugins.DependencyConstraints
+
+/*
+ * 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.
+ */
+
+apply from: "${rootDir}/${scriptDir}/standard-subproject-configuration.gradle"
+
+apply from: "${project.projectDir}/../gradle/publish-java.gradle"
+
+
+dependencies {
+    implementation(platform(project(':boms:geode-all-bom')))
+    compileOnly('org.jboss.modules:jboss-modules:' + DependencyConstraints.get("jboss-modules.version"))

Review comment:
       Fixed by removing the file and subproject.




----------------------------------------------------------------
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] yozaner1324 commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
yozaner1324 commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r582331095



##########
File path: geode-core/src/main/java/org/apache/geode/deployment/JarDeploymentService.java
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.deployment;
+
+import java.io.File;
+import java.util.List;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.api.ClusterManagementService;
+import org.apache.geode.management.configuration.Deployment;
+import org.apache.geode.services.result.ServiceResult;
+
+/**
+ * Implementations of {@link JarDeploymentService} will be responsible for deploying jars into Geode
+ * from both gfsh and the {@link ClusterManagementService}.
+ *
+ * @since Geode 1.15
+ */
+@Experimental
+public interface JarDeploymentService {

Review comment:
       Fixed.




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-785430978


   This pull request **introduces 1 alert** and **fixes 1** when merging c6a2613128e7c6a44ee1c541c2cff6017dbbe888 into e08cad4d7198f9bd82614ca9b7e631df36b38757 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-7cfdce297dd860dae15d075b4892dc4ed690505e)
   
   **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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r577953677



##########
File path: geode-core/build.gradle
##########
@@ -233,6 +233,8 @@ dependencies {
         exclude group: '*'
     }
 
+    runtimeOnly(project(':geode-deployment-legacy'))

Review comment:
       Why? Isn't this a cycle, between `geode-core` and `geode-deployment-legacy` ? And what symbols does `geode-core` need at runtime, that it does not need to compile? I am suspicious that `geode-core` might be getting `geode-deployment-legacy` at compile-time as a transitive dependency.

##########
File path: geode-deployment-legacy/build.gradle
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.
+ */
+
+apply from: "${project.projectDir}/../gradle/standard-subproject-configuration.gradle"
+
+apply from: "${project.projectDir}/../gradle/publish-java.gradle"
+apply from: "${project.projectDir}/../gradle/warnings.gradle"
+
+dependencies {
+    implementation(platform(project(':boms:geode-all-bom')))
+    compileOnly(project(':geode-core'))

Review comment:
       `LegacyJarDeploymentService` is implementing `JarDeploymentService` from `geode-core`. That means `geode-core` is needed at runtime, as well as compile time. I recommend this be `implementation` not `compileOnly`




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5989:
URL: https://github.com/apache/geode/pull/5989#issuecomment-785553351


   This pull request **introduces 1 alert** and **fixes 1** when merging b2e06926c4201ec2c8fbabd2b059a70c6e545a48 into e08cad4d7198f9bd82614ca9b7e631df36b38757 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-5db88797749660f4d5fc3bddc05d21d0553b977d)
   
   **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] DonalEvans commented on a change in pull request #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r578655302



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
##########
@@ -94,26 +92,16 @@ public void deployJarsReceivedFromClusterConfiguration(ConfigurationResponse res
             .flatMap(Set::stream)
             .collect(Collectors.toList());
 
-    if (jarFileNames != null && !jarFileNames.isEmpty()) {
+    if (!jarFileNames.isEmpty()) {
       logger.info("Got response with jars: {}", jarFileNames.stream().collect(joining(",")));

Review comment:
       This line can be replaced with `logger.info("Got response with jars: {}", String.join(",", jarFileNames));`




----------------------------------------------------------------
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 #5989: GEODE-8905: Introduce JarDeploymentService

Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on a change in pull request #5989:
URL: https://github.com/apache/geode/pull/5989#discussion_r581448170



##########
File path: geode-deployment-legacy/src/main/java/org/apache/geode/deployment/impl/legacy/LegacyJarDeploymentService.java
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.deployment.impl.legacy;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.time.Instant;
+import java.util.Collections;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+
+import org.apache.logging.log4j.Logger;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.deployment.JarDeploymentService;
+import org.apache.geode.internal.DeployedJar;
+import org.apache.geode.internal.JarDeployer;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.management.configuration.Deployment;
+import org.apache.geode.services.result.ServiceResult;
+import org.apache.geode.services.result.impl.Failure;
+import org.apache.geode.services.result.impl.Success;
+
+/**
+ * Implementation of {@link JarDeploymentService} that encpsulates the standard method of deploying
+ * jars by delegating to the {@link JarDeployer}.
+ *
+ * @since Geode 1.15
+ */
+@Experimental
+public class LegacyJarDeploymentService implements JarDeploymentService {
+
+  private static final Logger logger = LogService.getLogger();
+
+  private final Map<String, Deployment> deployments;
+  private JarDeployer jarDeployer;

Review comment:
       Given the way the legacy works, this JarDeployer can be replaced with another JarDeployer. Once again, we are just wrapping the existing legacy approach with a service interface. We aren't changing how the existing code works




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