[GitHub] [ignite] sergey-chugunov-1985 commented on a change in pull request #9020: IGNITE-14131 IgniteCompute tasks with same name, running from one nod…

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

sergey-chugunov-1985 commented on a change in pull request #9020:
URL: https://github.com/apache/ignite/pull/9020#discussion_r623652900



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java
##########
@@ -356,20 +355,22 @@ private GridDeployment deploy(DeploymentMode depMode, ClassLoader ldr, Class<?>
             if (clsLdr.getClass().equals(GridDeploymentClassLoader.class))
                 clsLdr = clsLdr.getParent();
 
-            GridDeployment dep = null;
+            if (spi.register(clsLdr, cls) && log.isDebugEnabled()) {

Review comment:
       The same about mixed checks as above.

##########
File path: modules/core/src/main/java/org/apache/ignite/IgniteCompute.java
##########
@@ -371,6 +371,9 @@ public void affinityRun(@NotNull Collection<String> cacheNames, int partId, Igni
      * <p>
      * If task for given name has not been deployed yet, then {@code taskName} will be
      * used as task class name to auto-deploy the task (see {@link #localDeployTask(Class, ClassLoader)} method).
+     * <p>
+     * If there are more one class deployed with the same name this method will execute the lasts one deployed of them.
+     * This method has no guarantees if it invokes where classes with the same names deployed from different threads.

Review comment:
       I suggest to reformulate javadoc like this:
   
   `If class with the same name was deployed more than once, the last deployed version is used.
   If method is called when other threads are deploying other versions of class with the same name there are no guarantees which version of the class will be executed.`

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java
##########
@@ -154,74 +154,64 @@
             return dep;
         }
 
-        DeploymentResource rsrc = spi.findResource(alias);
+        if (meta.classLoader() == null) {
+            DeploymentResource rsrc = spi.findResource(alias);
 
-        if (rsrc != null) {
-            dep = deploy(ctx.config().getDeploymentMode(), rsrc.getClassLoader(), rsrc.getResourceClass(), alias,
-                meta.record());
+            if (rsrc != null) {
+                dep = deploy(ctx.config().getDeploymentMode(), rsrc.getClassLoader(), rsrc.getResourceClass(), alias,
+                    meta.record());
 
-            assert dep != null;
+                assert dep != null;
 
-            if (log.isDebugEnabled())
-                log.debug("Acquired deployment class from SPI: " + dep);
-        }
-        // Auto-deploy.
-        else {
-            ClassLoader ldr = meta.classLoader();
-
-            if (ldr == null) {
-                ldr = Thread.currentThread().getContextClassLoader();
-
-                // Safety.
-                if (ldr == null)
-                    ldr = U.resolveClassLoader(ctx.config());
-            }
-
-            if (ldr instanceof GridDeploymentClassLoader) {
                 if (log.isDebugEnabled())
-                    log.debug("Skipping local auto-deploy (nested execution) [ldr=" + ldr + ", meta=" + meta + ']');
+                    log.debug("Acquired deployment class from SPI: " + dep);
 
-                return null;
+                return dep;
             }
+        }
 
-            while (true) {
-                try {
-                    // Check that class can be loaded.
-                    String clsName = meta.className();
+        // Auto-deploy.
+        ClassLoader ldr = meta.classLoader();
 
-                    Class<?> cls = U.forName(clsName != null ? clsName : alias, ldr);
+        if (ldr == null) {
+            ldr = Thread.currentThread().getContextClassLoader();
 
-                    spi.register(ldr, cls);
+            // Safety.
+            if (ldr == null)
+                ldr = U.resolveClassLoader(ctx.config());
+        }
 
-                    rsrc = spi.findResource(cls.getName());
+        if (ldr instanceof GridDeploymentClassLoader) {
+            if (log.isDebugEnabled())
+                log.debug("Skipping local auto-deploy (nested execution) [ldr=" + ldr + ", meta=" + meta + ']');
 
-                    if (rsrc != null && rsrc.getResourceClass().equals(cls)) {
-                        if (log.isDebugEnabled())
-                            log.debug("Retrieved auto-loaded resource from spi: " + rsrc);
+            return null;
+        }
 
-                        dep = deploy(ctx.config().getDeploymentMode(), ldr, cls, meta.alias(), meta.record());
+        try {
+            // Check that class can be loaded.
+            String clsName = meta.className();
 
-                        if (dep != null)
-                            return dep;
-                    }
-                    else {
-                        U.warn(log, "Failed to find resource from deployment SPI even after registering: " + meta);
+            Class<?> cls = U.forName(clsName != null ? clsName : alias, ldr);
 
-                        return null;
-                    }
-                }
-                catch (ClassNotFoundException ignored) {
-                    if (log.isDebugEnabled())
-                        log.debug("Failed to load class for local auto-deployment [ldr=" + ldr + ", meta=" + meta + ']');
+            if (spi.register(ldr, cls) && log.isDebugEnabled()) {

Review comment:
       Mixing business logic and logging checks seems strange to me, I would rather use two more lines of code but put `debugEnabled` check inside the main `if` block.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org