IGNITE-27871 Improve deployment lookup to reduce deploy() contention … by oleg-vlsk · Pull Request #12760 · apache/ignite

oleg-vlsk · 2026-02-17T23:16:05Z

…for locally available tasks with peerClassLoadingEnabled=true

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

There is a single JIRA ticket related to the pull request.
The web-link to the pull request is attached to the JIRA ticket.
The JIRA ticket has the Patch Available state.
The pull request body describes changes that have been made.
The description explains WHAT and WHY was made instead of HOW.
The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
A reviewer has been mentioned through the JIRA comments
(see the Maintainers list)
The pull request has been checked by the Teamcity Bot and
the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.

alex-plekhanov · 2026-02-18T07:39:04Z

    P2PClassLoadingFailureHandlingTest.class,
-    P2PClassLoadingIssuesTest.class
+    P2PClassLoadingIssuesTest.class,
+    GridDeploymentLocalStoreReuseTest.class


Add comma to the end of line please (to reduce conflicts on merge)

alex-plekhanov · 2026-02-18T12:47:24Z

+            CompletableFuture<T2<UUID, Set<UUID>>> fut = client.compute()
+                .withTimeout(timeout).
+                    <T2<UUID, Set<UUID>>, T2<UUID, Set<UUID>>>executeAsync2(TestTask.class.getName(), null)
+                .toCompletableFuture();
+
+            try {
+                fut.get();


client.compute().execute(TestTask.class.getName(), null);

Used this snippet, thank you.

alex-plekhanov · 2026-02-18T12:52:18Z

+            List<IgniteInternalFuture<Void>> futs = new ArrayList<>(CLIENT_CNT);
+
+            for (IgniteClient client : clients)
+                futs.add(runAsync(() -> executeTasksOnClient(client, EXEC_CNT, 5_000L)));
+
+            waitForAllFutures(futs.toArray(new IgniteInternalFuture[0]));


runMultiThreaded(i -> executeTasksOnClient(clients.get(i), EXEC_CNT), CLIENT_CNT, "worker");

I decided not to go for multi-threaded execution as the perpose of the test is to verify certain behaviour during subsequent executions of the same task. So I ended up using a simple for-loop.

alex-plekhanov · 2026-02-18T12:54:07Z

+            ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+            for (int i = 0; i < CLIENT_CNT; i++)
+                clients.add(startClient(allServerNodes));


You can connect to any server node,it's not necessary to provide all nodes, one is enough, i.e. clients.add(startClient(0));

alex-plekhanov · 2026-02-18T13:49:00Z

+    /** */
+    private static class DeploymentListeningLogger extends ListeningTestLogger {
+        /** */
+        private final ConcurrentLinkedQueue<String> depNotFound = new ConcurrentLinkedQueue<>();
+
+        /** */
+        public DeploymentListeningLogger(IgniteLogger log) {
+            super(log);
+        }
+
+        /** {@inheritDoc} */
+        @Override public void debug(String msg) {
+            if (msg.contains("Deployment was not found for class with specific class loader"))
+                depNotFound.add(msg);
+
+            super.debug(msg);
+        }
+
+        /** {@inheritDoc} */
+        @Override public ListeningTestLogger getLogger(Object ctgr) {
+            return this;
+        }
+
+        /** */
+        public List<String> depNotFound() {
+            return depNotFound.stream().collect(Collectors.toUnmodifiableList());
+        }
+    }
+}


It's incorrect usage of listening logger, all you need is register listener like:

LogListener lsnr = LogListener.matches(notFoundMsg).times(CLIENT_CNT).build(); listeningTestLog.registerListener(lsnr);

listeningTestLog should be created on top of standard logger, for example:

setLoggerDebugLevel(); listeningTestLog = new ListeningTestLogger(log);

And passed to ignite configuration. No need for logger for each node.

alex-plekhanov · 2026-02-19T08:08:37Z

        meta.alias(rsrcName);
        meta.className(clsName);
        meta.senderNodeId(ctx.localNodeId());
+        meta.classLoader(ldr);


Setting classloader disables deployment SPI as far as I understand. See https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java#L174

Moved the local app classloader check to GridDeploymentLocalStore#deployment so that in the initial call the meta does not contains classloader.

alex-plekhanov · 2026-02-19T08:35:16Z

    private final ConcurrentMap<String, Deque<GridDeployment>> cache = new ConcurrentHashMap<>();

+    /** Deployment cache by classloader. */
+    private final ConcurrentMap<ClassLoader, Deque<GridDeployment>> cacheByLdr = new ConcurrentHashMap<>();


cacheByLdr always used under the lock mux, no ConcurrentMap overhead required here.
Also maybe it worth to use IdentityHashMap in case someone redefine classloader's equals() in a wrong way.

Done, thank you for the hint.

alex-plekhanov · 2026-02-19T08:57:45Z


-                            dep = d;
+                    for (GridDeployment d : depsByLdr) {
+                        if (!d.undeployed() && d.classLoader() == ldr) {


If it's undeployed, it's cleaned from cache, how we can find it?
Why do we need to check classloader if we put in cache only items with exactly this classloader?

Yes, those were 'extra safety' checks. Changed the lookup logic altogether (see below).

alex-plekhanov · 2026-02-19T09:00:27Z

+                        dep = candidate;
+                    }
+                }
+                else {


Do we still need this check? If deployment not found by classloader in classloader cache it can't be found in aliases cache. We preserve both caches synchronized and modify it only under the lock.

Removed this else block.

alex-plekhanov · 2026-02-19T09:01:57Z

-                        if (d.classLoader() == ldr) {
-                            // Cache class and alias.
-                            fireEvt = d.addDeployedClass(cls, alias);
+                Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);


Looks like it's one-to-one relation for deployment and classloader. Did I miss something?

Not necessarily. In GridDeploymentLocalStore#cache we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). Most recent deployment are added to the beginning of the queue (the addFirst() call in GridDeploymentLocalStore#deploy).

I'm talking about cacheByLdr, not cache. For cacheByLdr it looks like only one deployment is possible for one classloader.

alex-plekhanov · 2026-02-22T07:24:27Z

+                    ClassLoader ldr = Thread.currentThread().getContextClassLoader();
+
+                    if (ldr == null)
+                        ldr = U.resolveClassLoader(ctx.config());


Let's move ldr initialization outside the loop.

Just add || dep.classLoader() == ldr to the if condition

Moved classloader initialization outside the for loop, but kept dep.classLoader() == ldr as a separate if-block to be able to log correct trace messages. As I understand, if we add the || dep.classLoader() == ldr condition to the initial if-block, the trace message will contain either null as classloader id or incorrect classloader id. As well as it won’t be obvious from such message that the local app classloader was used in this case.

alex-plekhanov · 2026-02-22T07:34:34Z

-                        if (d.classLoader() == ldr) {
-                            // Cache class and alias.
-                            fireEvt = d.addDeployedClass(cls, alias);
+                Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);


I'm talking about cacheByLdr, not cache. For cacheByLdr it looks like only one deployment is possible for one classloader.

alex-plekhanov · 2026-02-22T07:36:46Z

+        assertTrue(lsnr0.check(5_000));
+        assertTrue(lsnr1.check(5_000));


Why do we need to wait here? As far as I understand here strict happens-before between task completion and log message.

Removed timeout.

oleg-vlsk · 2026-02-25T00:25:54Z

@alex-plekhanov Regarding you comment:

"I'm talking about cacheByLdr, not cache. For cacheByLdr it looks like only one deployment is possible for one classloader."

I added:

 private final Map<ClassLoader, GridDeployment> depsByLdr = new IdentityHashMap<>();

to map classloader to a specific deployment. But I laso kept:

private final Map<ClassLoader, Deque<GridDeployment>> cacheByLdr = new IdentityHashMap<>();

to have quick access to all deployments with the given classloader so we can add them all to cache in order to preserve the current contract:

if (cachedDeps != null) {
    assert dep != null;

    cache.put(alias, cachedDeps);

    if (!cls.getName().equals(alias)) 
        cache.put(cls.getName(), cachedDeps);
    
    ...
}

alex-plekhanov · 2026-04-02T12:07:59Z

                }
            }
+
+            if (dep != null && !dep.undeployed()) {


dep.undeployed() should be checked inside the loop, or we stop searching after first undeployed deployment

Added the dep.undeployed check to the loop.

alex-plekhanov · 2026-04-02T12:08:22Z

+                dep = deps.get(meta.classLoader());

-                    return dep;
+            if (dep == null && meta.classLoaderId() != null) {


(dep == null || dep.undeployed) && ...

alex-plekhanov · 2026-04-02T12:20:14Z

     */
    private void undeploy(ClassLoader ldr) {
-        Collection<GridDeployment> doomed = new HashSet<>();
+        Collection<GridDeployment> doomed = U.newIdentityHashSet();


Looks like there can be only one doomed instance? Or I miss something?

Refactored the undeploy() method based on the 1:1 classloder to deployment relation.

alex-plekhanov · 2026-04-02T12:25:32Z

        synchronized (mux) {
-            for (Iterator<Deque<GridDeployment>> i1 = cache.values().iterator(); i1.hasNext();) {
-                Deque<GridDeployment> deps = i1.next();
+            for (Iterator<Map<ClassLoader, GridDeployment>> i1 = depsByAlias.values().iterator(); i1.hasNext();) {


Maybe reverse delete from caches order? This will also allow to call undeploy only once per instance. Like this:

GridDeployment dep = depByLdr.remove(ldr); if (dep != null) { dep.undeploy(); doomed.add(dep); if (log.isInfoEnabled()) log.info("Removed undeployed class: " + dep); for (Iterator<Map<ClassLoader, GridDeployment>> i1 = depsByAlias.values().iterator(); i1.hasNext(); ) { Map<ClassLoader, GridDeployment> deps = i1.next(); deps.remove(ldr); if (deps.isEmpty()) i1.remove(); } }

Done as part of refactoring.

…for locally available tasks with peerClassLoadingEnabled=true

…calStore#deployment, correct cache lookup mechanism in GridDeploymentLocalStore#deploy, simplify GridDeploymentLocalStoreReuseTest#testNoExcessiveLocalDeploymentCacheMisses

…/secondary indexes

…testCheckTaskClassloaderCache with regard to using GridDeploymentLocalStore#depsByAlias via reflection

…loymentLocalStore#undeploy

sonarqubecloud · 2026-04-03T05:26:28Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

alex-plekhanov reviewed Feb 19, 2026

View reviewed changes

alex-plekhanov reviewed Feb 22, 2026

View reviewed changes

oleg-vlsk force-pushed the ignite-27871 branch from a243265 to d3f50e1 Compare March 30, 2026 00:41

alex-plekhanov reviewed Apr 2, 2026

View reviewed changes

Valuyskiy.O.Y added 9 commits April 3, 2026 08:43

IGNITE-27871 Improve deployment lookup to reduce deploy() contention …

aa75f43

…for locally available tasks with peerClassLoadingEnabled=true

IGNITE-27871 Move local classloader checkup logic to GridDeploymentLo…

74d1db1

…calStore#deployment, correct cache lookup mechanism in GridDeploymentLocalStore#deploy, simplify GridDeploymentLocalStoreReuseTest#testNoExcessiveLocalDeploymentCacheMisses

IGNITE-27871 Remove GridDeploymentLocalStore#depByLdr

113da50

IGNITE-27871 Add corrections as per review

49215a9

IGNITE-27871 Add corrections as per review

8bc1f4d

IGNITE-27871 Refactor GridDeploymentLocalStore caching to use primary…

d3e4306

…/secondary indexes

IGNITE-27871 Add minor self-review corrections

35f8e02

IGNITE-27871 Add corrections to GridDifferentLocalDeploymentSelfTest#…

360c87d

…testCheckTaskClassloaderCache with regard to using GridDeploymentLocalStore#depsByAlias via reflection

IGNITE-27871 Add minor corrections as per review and refactor GridDep…

f01f5b1

…loymentLocalStore#undeploy

oleg-vlsk force-pushed the ignite-27871 branch from 76d8e06 to f01f5b1 Compare April 2, 2026 22:44

alex-plekhanov approved these changes Apr 3, 2026

View reviewed changes

asf-gitbox-commits closed this in 4984f18 Apr 7, 2026

		assertTrue(lsnr0.check(5_000));
		assertTrue(lsnr1.check(5_000));

Conversation

oleg-vlsk commented Feb 17, 2026

The Contribution Checklist

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-vlsk Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-vlsk Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-vlsk commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-vlsk Feb 21, 2026 •

edited

Loading

oleg-vlsk Feb 21, 2026 •

edited

Loading

oleg-vlsk commented Feb 25, 2026 •

edited

Loading