You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "oleg-vlsk (via GitHub)" <gi...@apache.org> on 2026/02/17 23:16:07 UTC
[PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
oleg-vlsk opened a new pull request, #12760:
URL: https://github.com/apache/ignite/pull/12760
…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](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836663472
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
+
+ List<String> allNotFound = new ArrayList<>();
+
+ for (DeploymentListeningLogger log : logs)
+ allNotFound.addAll(log.depNotFound());
+
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ assertEquals(CLIENT_CNT, Collections.frequency(allNotFound, notFoundMsg));
+ }
+ finally {
+ clients.forEach(IgniteClient::close);
+ }
+ }
+
+ /** */
+ private static void executeTasksOnClient(IgniteClient client, int cnt, long timeout) {
+ for (int i = 0; i < cnt; i++) {
+ 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();
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In GridDeploymentLocalStore#cache we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). But I agree with you that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
But at the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of cache, where each alias/class is mapped to a colleciton of deployments associated with the same classloader.
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836670764
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12760:
URL: https://github.com/apache/ignite/pull/12760#issuecomment-4181954781
## [](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12760) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=12760&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=12760&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=12760&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=12760&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=12760&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12760)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In `GridDeploymentLocalStore#cache` we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). However, according to the current `GridDeploymentLocalStore#deploy` code in master there is no guarantee that all deployments in `cache` will have the same classloader (the classloader `==` check in the lookup for loop).
But I agree with you that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
But at the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of cache, where each alias/class is mapped to a colleciton of deployments associated with the same classloader.
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836684679
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
- cachedDeps = deps;
+ if (depsByLdr != null) {
+ GridDeployment candidate = null;
- dep = d;
+ for (GridDeployment d : depsByLdr) {
+ if (!d.undeployed() && d.classLoader() == ldr) {
+ candidate = d;
break;
}
}
- if (cachedDeps != null)
- break;
+ if (candidate != null) {
+ fireEvt = candidate.addDeployedClass(cls, alias);
+
+ cachedDeps = depsByLdr;
+
+ dep = candidate;
+ }
+ }
+ else {
Review Comment:
Removed this else 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "asf-gitbox-commits (via GitHub)" <gi...@apache.org>.
asf-gitbox-commits closed pull request #12760: IGNITE-27871 Improve deployment lookup to reduce deploy() contention …
URL: https://github.com/apache/ignite/pull/12760
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r3030708861
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -248,22 +235,55 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
* @return Deployment.
*/
@Nullable private GridDeployment deployment(final GridDeploymentMetadata meta) {
- Deque<GridDeployment> deps = cache.get(meta.alias());
+ Map<ClassLoader, GridDeployment> deps;
+
+ synchronized (mux) {
+ Map<ClassLoader, GridDeployment> cached = depsByAlias.get(meta.alias());
+
+ deps = cached == null ? null : new IdentityHashMap<>(cached);
+ }
if (deps != null) {
- for (GridDeployment dep : deps) {
- if (dep.undeployed())
- continue;
+ GridDeployment dep = null;
- // local or remote deployment.
- if (dep.classLoaderId() == meta.classLoaderId() || dep.classLoader() == meta.classLoader()) {
- if (log.isTraceEnabled())
- log.trace("Deployment was found for class with specific class loader [alias=" + meta.alias() +
- ", clsLdrId=" + meta.classLoaderId() + "]");
+ if (meta.classLoader() != null)
+ dep = deps.get(meta.classLoader());
- return dep;
+ if (dep == null && meta.classLoaderId() != null) {
+ for (GridDeployment d : deps.values()) {
+ if (d.classLoaderId().equals(meta.classLoaderId())) {
+ dep = d;
+
+ break;
+ }
}
}
+
+ if (dep != null && !dep.undeployed()) {
Review Comment:
Added the `dep.undeployed` check to the loop.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -248,22 +235,55 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
* @return Deployment.
*/
@Nullable private GridDeployment deployment(final GridDeploymentMetadata meta) {
- Deque<GridDeployment> deps = cache.get(meta.alias());
+ Map<ClassLoader, GridDeployment> deps;
+
+ synchronized (mux) {
+ Map<ClassLoader, GridDeployment> cached = depsByAlias.get(meta.alias());
+
+ deps = cached == null ? null : new IdentityHashMap<>(cached);
+ }
if (deps != null) {
- for (GridDeployment dep : deps) {
- if (dep.undeployed())
- continue;
+ GridDeployment dep = null;
- // local or remote deployment.
- if (dep.classLoaderId() == meta.classLoaderId() || dep.classLoader() == meta.classLoader()) {
- if (log.isTraceEnabled())
- log.trace("Deployment was found for class with specific class loader [alias=" + meta.alias() +
- ", clsLdrId=" + meta.classLoaderId() + "]");
+ if (meta.classLoader() != null)
+ dep = deps.get(meta.classLoader());
- return dep;
+ if (dep == null && meta.classLoaderId() != null) {
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836672419
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
+
+ List<String> allNotFound = new ArrayList<>();
+
+ for (DeploymentListeningLogger log : logs)
+ allNotFound.addAll(log.depNotFound());
+
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ assertEquals(CLIENT_CNT, Collections.frequency(allNotFound, notFoundMsg));
+ }
+ finally {
+ clients.forEach(IgniteClient::close);
+ }
+ }
+
+ /** */
+ private static void executeTasksOnClient(IgniteClient client, int cnt, long timeout) {
+ for (int i = 0; i < cnt; i++) {
+ 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();
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+ }
+
+ /** */
+ 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());
+ }
+ }
+}
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836682889
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
- cachedDeps = deps;
+ if (depsByLdr != null) {
+ GridDeployment candidate = null;
- dep = d;
+ for (GridDeployment d : depsByLdr) {
+ if (!d.undeployed() && d.classLoader() == ldr) {
Review Comment:
Yes, those were 'extra safety' checks. Changed the lookup logic altogether (see below).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In `GridDeploymentLocalStore#cache` we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). However, according to the current `GridDeploymentLocalStore#deploy` code in master it looks like there is no guarantee that all deployments in `cache` will have the same classloader (hence the classloader `==` check in the lookup for-loop).
But I agree with you that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
But at the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of `cache`, where each alias/class is mapped to a colleciton of deployments associated with the same classloader.
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2837292266
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -263,6 +267,20 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
return dep;
}
+ else {
+ ClassLoader ldr = Thread.currentThread().getContextClassLoader();
+
+ if (ldr == null)
+ ldr = U.resolveClassLoader(ctx.config());
Review Comment:
1. Let's move ldr initialization outside the loop.
2. Just add `|| dep.classLoader() == ldr` to the `if` condition
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.config.Configurator;
+import org.apache.logging.log4j.core.config.LoggerConfig;
+import org.junit.Test;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final String LOG_NAME = "org.apache.ignite";
+
+ /** */
+ private static final int EXEC_CNT = 3;
+
+ /** */
+ private static Level initLogLevel;
+
+ /** */
+ private static ListeningTestLogger testLog;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ LoggerConfig logCfg = LoggerContext.getContext(false).getConfiguration().getLoggerConfig(LOG_NAME);
+
+ initLogLevel = logCfg.getLevel();
+
+ Configurator.setLevel(LOG_NAME, Level.TRACE);
+
+ testLog = new ListeningTestLogger(log);
+
+ startGrids(2);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ Configurator.setLevel(LOG_NAME, initLogLevel);
+
+ initLogLevel = null;
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once upon initial task execution. The trace-level "deployment
+ * was found for class with the local app class loader" messages must be present in the output marking the subsequent
+ * task executions without cache misses.
+ */
+ @Test
+ public void testNoExcessiveLocalDeploymentCacheMisses() throws Exception {
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ String foundLocLdrMsg = String.format(
+ "Deployment was found for class with the local app class loader [alias=%s]", taskClsName);
+
+ LogListener lsnr0 = LogListener.matches(notFoundMsg).times(1).build();
+ LogListener lsnr1 = LogListener.matches(foundLocLdrMsg).atLeast(EXEC_CNT - 1).build();
+
+ testLog.registerListener(lsnr0);
+ testLog.registerListener(lsnr1);
+
+ try (IgniteClient client = startClient(0)) {
+ for (int i = 0; i < EXEC_CNT; i++)
+ client.compute().execute(TestTask.class.getName(), null);
+ }
+
+ assertTrue(lsnr0.check(5_000));
+ assertTrue(lsnr1.check(5_000));
Review Comment:
Why do we need to wait here? As far as I understand here strict happens-before between task completion and log message.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
I'm talking about `cacheByLdr`, not `cache`. For `cacheByLdr` it looks like only one deployment is possible for one 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836679181
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentManager.java:
##########
@@ -391,13 +391,16 @@ private GridDeployment checkDeployment(GridDeployment deployment, String store)
String clsName = lambdaEnclosingClsName == null ? rsrcName : lambdaEnclosingClsName;
+ ClassLoader ldr = Thread.currentThread().getContextClassLoader();
+
GridDeploymentMetadata meta = new GridDeploymentMetadata();
meta.record(true);
meta.deploymentMode(ctx.config().getDeploymentMode());
meta.alias(rsrcName);
meta.className(clsName);
meta.senderNodeId(ctx.localNodeId());
+ meta.classLoader(ldr);
Review Comment:
Moved the local app classloader check to `GridDeploymentLocalStore#deployment` so that in the initial call the meta does not contains 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In `GridDeploymentLocalStore#cache` we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). However, according to the current `GridDeploymentLocalStore#deploy` code in master it looks like there is no guarantee that all deployments in `cache` will have the same classloader (hence the classloader `==` check in the lookup for-loop).
But I agree with you that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
But at the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of `cache`, where each alias/class is mapped to a colleciton of associated deployments.
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2826341664
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -63,6 +63,9 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
/** Deployment cache by class name. */
private final ConcurrentMap<String, Deque<GridDeployment>> cache = new ConcurrentHashMap<>();
+ /** Deployment cache by classloader. */
+ private final ConcurrentMap<ClassLoader, Deque<GridDeployment>> cacheByLdr = new ConcurrentHashMap<>();
Review Comment:
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.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentManager.java:
##########
@@ -391,13 +391,16 @@ private GridDeployment checkDeployment(GridDeployment deployment, String store)
String clsName = lambdaEnclosingClsName == null ? rsrcName : lambdaEnclosingClsName;
+ ClassLoader ldr = Thread.currentThread().getContextClassLoader();
+
GridDeploymentMetadata meta = new GridDeploymentMetadata();
meta.record(true);
meta.deploymentMode(ctx.config().getDeploymentMode());
meta.alias(rsrcName);
meta.className(clsName);
meta.senderNodeId(ctx.localNodeId());
+ meta.classLoader(ldr);
Review Comment:
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
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
+
+ List<String> allNotFound = new ArrayList<>();
+
+ for (DeploymentListeningLogger log : logs)
+ allNotFound.addAll(log.depNotFound());
+
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ assertEquals(CLIENT_CNT, Collections.frequency(allNotFound, notFoundMsg));
+ }
+ finally {
+ clients.forEach(IgniteClient::close);
+ }
+ }
+
+ /** */
+ private static void executeTasksOnClient(IgniteClient client, int cnt, long timeout) {
+ for (int i = 0; i < cnt; i++) {
+ 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();
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+ }
+
+ /** */
+ 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());
+ }
+ }
+}
Review Comment:
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.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
- cachedDeps = deps;
+ if (depsByLdr != null) {
+ GridDeployment candidate = null;
- dep = d;
+ for (GridDeployment d : depsByLdr) {
+ if (!d.undeployed() && d.classLoader() == ldr) {
Review Comment:
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?
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Looks like it's one-to-one relation for deployment and classloader. Did I miss something?
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
Review Comment:
You can connect to any server node,it's not necessary to provide all nodes, one is enough, i.e. `clients.add(startClient(0));`
##########
modules/core/src/test/java/org/apache/ignite/testsuites/IgniteP2PSelfTestSuite.java:
##########
@@ -82,7 +83,8 @@
GridDifferentLocalDeploymentSelfTest.class,
P2PUnsupportedClassVersionTest.class,
P2PClassLoadingFailureHandlingTest.class,
- P2PClassLoadingIssuesTest.class
+ P2PClassLoadingIssuesTest.class,
+ GridDeploymentLocalStoreReuseTest.class
Review Comment:
Add comma to the end of line please (to reduce conflicts on merge)
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
Review Comment:
```
runMultiThreaded(i -> executeTasksOnClient(clients.get(i), EXEC_CNT), CLIENT_CNT, "worker");
```
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
+
+ List<String> allNotFound = new ArrayList<>();
+
+ for (DeploymentListeningLogger log : logs)
+ allNotFound.addAll(log.depNotFound());
+
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ assertEquals(CLIENT_CNT, Collections.frequency(allNotFound, notFoundMsg));
+ }
+ finally {
+ clients.forEach(IgniteClient::close);
+ }
+ }
+
+ /** */
+ private static void executeTasksOnClient(IgniteClient client, int cnt, long timeout) {
+ for (int i = 0; i < cnt; i++) {
+ 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();
Review Comment:
```
client.compute().execute(TestTask.class.getName(), null);
```
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
- cachedDeps = deps;
+ if (depsByLdr != null) {
+ GridDeployment candidate = null;
- dep = d;
+ for (GridDeployment d : depsByLdr) {
+ if (!d.undeployed() && d.classLoader() == ldr) {
+ candidate = d;
break;
}
}
- if (cachedDeps != null)
- break;
+ if (candidate != null) {
+ fireEvt = candidate.addDeployedClass(cls, alias);
+
+ cachedDeps = depsByLdr;
+
+ dep = candidate;
+ }
+ }
+ else {
Review Comment:
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.
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
+
+ List<String> allNotFound = new ArrayList<>();
+
+ for (DeploymentListeningLogger log : logs)
+ allNotFound.addAll(log.depNotFound());
+
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ assertEquals(CLIENT_CNT, Collections.frequency(allNotFound, notFoundMsg));
Review Comment:
It's not a correct check. You get CLIENT_CNT occurances here just becouse clients start tasks concurrently and you enter the same section by different threads before class was deployed to cluster. In case of any delay test become flaky. For example, if you add something like
```
clients.get(0).compute().execute(TestTask.class.getName(), null);
doSleep(1000);
```
Before massive client tasks start, you will get just one occurance of message independent of clients count.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2850143086
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -263,6 +267,20 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
return dep;
}
+ else {
+ ClassLoader ldr = Thread.currentThread().getContextClassLoader();
+
+ if (ldr == null)
+ ldr = U.resolveClassLoader(ctx.config());
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836680261
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -63,6 +63,9 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
/** Deployment cache by class name. */
private final ConcurrentMap<String, Deque<GridDeployment>> cache = new ConcurrentHashMap<>();
+ /** Deployment cache by classloader. */
+ private final ConcurrentMap<ClassLoader, Deque<GridDeployment>> cacheByLdr = new ConcurrentHashMap<>();
Review Comment:
Done, thank you for the hint.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In `GridDeploymentLocalStore#cache` we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots).
But it seems that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
At the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of `cache`, where each alias/class is mapped to a colleciton of associated deployments.
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836780780
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
+
+ List<String> allNotFound = new ArrayList<>();
+
+ for (DeploymentListeningLogger log : logs)
+ allNotFound.addAll(log.depNotFound());
+
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ assertEquals(CLIENT_CNT, Collections.frequency(allNotFound, notFoundMsg));
Review Comment:
Changed the test logic.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r3027661158
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -248,22 +235,55 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
* @return Deployment.
*/
@Nullable private GridDeployment deployment(final GridDeploymentMetadata meta) {
- Deque<GridDeployment> deps = cache.get(meta.alias());
+ Map<ClassLoader, GridDeployment> deps;
+
+ synchronized (mux) {
+ Map<ClassLoader, GridDeployment> cached = depsByAlias.get(meta.alias());
+
+ deps = cached == null ? null : new IdentityHashMap<>(cached);
+ }
if (deps != null) {
- for (GridDeployment dep : deps) {
- if (dep.undeployed())
- continue;
+ GridDeployment dep = null;
- // local or remote deployment.
- if (dep.classLoaderId() == meta.classLoaderId() || dep.classLoader() == meta.classLoader()) {
- if (log.isTraceEnabled())
- log.trace("Deployment was found for class with specific class loader [alias=" + meta.alias() +
- ", clsLdrId=" + meta.classLoaderId() + "]");
+ if (meta.classLoader() != null)
+ dep = deps.get(meta.classLoader());
- return dep;
+ if (dep == null && meta.classLoaderId() != null) {
Review Comment:
(dep == null || dep.undeployed) && ...
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -248,22 +235,55 @@ class GridDeploymentLocalStore extends GridDeploymentStoreAdapter {
* @return Deployment.
*/
@Nullable private GridDeployment deployment(final GridDeploymentMetadata meta) {
- Deque<GridDeployment> deps = cache.get(meta.alias());
+ Map<ClassLoader, GridDeployment> deps;
+
+ synchronized (mux) {
+ Map<ClassLoader, GridDeployment> cached = depsByAlias.get(meta.alias());
+
+ deps = cached == null ? null : new IdentityHashMap<>(cached);
+ }
if (deps != null) {
- for (GridDeployment dep : deps) {
- if (dep.undeployed())
- continue;
+ GridDeployment dep = null;
- // local or remote deployment.
- if (dep.classLoaderId() == meta.classLoaderId() || dep.classLoader() == meta.classLoader()) {
- if (log.isTraceEnabled())
- log.trace("Deployment was found for class with specific class loader [alias=" + meta.alias() +
- ", clsLdrId=" + meta.classLoaderId() + "]");
+ if (meta.classLoader() != null)
+ dep = deps.get(meta.classLoader());
- return dep;
+ if (dep == null && meta.classLoaderId() != null) {
+ for (GridDeployment d : deps.values()) {
+ if (d.classLoaderId().equals(meta.classLoaderId())) {
+ dep = d;
+
+ break;
+ }
}
}
+
+ if (dep != null && !dep.undeployed()) {
Review Comment:
`dep.undeployed()` should be checked inside the loop, or we stop searching after first undeployed deployment
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -543,14 +535,16 @@ private String getAlias(GridDeployment dep, Class<?> cls) {
* @param ldr Class loader to undeploy.
*/
private void undeploy(ClassLoader ldr) {
- Collection<GridDeployment> doomed = new HashSet<>();
+ Collection<GridDeployment> doomed = U.newIdentityHashSet();
Review Comment:
Looks like there can be only one doomed instance? Or I miss something?
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -543,14 +535,16 @@ private String getAlias(GridDeployment dep, Class<?> cls) {
* @param ldr Class loader to undeploy.
*/
private void undeploy(ClassLoader ldr) {
- Collection<GridDeployment> doomed = new HashSet<>();
+ Collection<GridDeployment> doomed = U.newIdentityHashSet();
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();) {
Review Comment:
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();
}
}
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836670133
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836662577
##########
modules/core/src/test/java/org/apache/ignite/testsuites/IgniteP2PSelfTestSuite.java:
##########
@@ -82,7 +83,8 @@
GridDifferentLocalDeploymentSelfTest.class,
P2PUnsupportedClassVersionTest.class,
P2PClassLoadingFailureHandlingTest.class,
- P2PClassLoadingIssuesTest.class
+ P2PClassLoadingIssuesTest.class,
+ GridDeploymentLocalStoreReuseTest.class
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In `GridDeploymentLocalStore#cache` we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). However, according to the current `GridDeploymentLocalStore#deploy` code in master there is no guarantee that all deployments in `cache` will have the same classloader (the classloader `==` check in the lookup for-loop).
But I agree with you that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
But at the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of `cache`, where each alias/class is mapped to a colleciton of deployments associated with the same classloader.
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In GridDeploymentLocalStore#cache we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). But I agree with you that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
But at the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of cache, where each alias/class is mapped to a colleciton of deployments associated with the same 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on PR #12760:
URL: https://github.com/apache/ignite/pull/12760#issuecomment-3955541046
@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 agree, for the sole purpose of getting a certain deployment by its classloader we could simply use:
```
private final Map<ClassLoader, GridDeployment> cacheByLdr = new IdentityHashMap<>();
```
The reason I mapped classloader to all its deployments is because currently in `cache` we keep all deployments associated with the given classloader. Once we identify the target deployment, we need to add all associated deployments to `cache`:
```
if (cachedDeps != null) {
assert dep != null;
cache.put(alias, cachedDeps);
if (!cls.getName().equals(alias))
cache.put(cls.getName(), cachedDeps);
...
}
```
Which means that apart from
```
private final Map<ClassLoader, GridDeployment> cacheByLdr = new IdentityHashMap<>();
```
We would still need something like
```
private final Map<ClassLoader, Deque<GridDeployment>> depsByLdr = new IdentityHashMap<>();
```
to have quick access to all deployments with the given classloader so we can add all of them to `cache` in order to preserve the current contract.
But keeping both of those fields seems unnecessary, since peekFirst() from the queue is still O(1).
Unless there is another way of going about it, which I’m unaware of.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
Not necessarily. In `GridDeploymentLocalStore#cache` we can have several deployments with the same classloader associated with one alias/class name (see attached screenshots). However, according to the current `GridDeploymentLocalStore#deploy` code in master there is no guarantee that all deployments in `cache` will have the same classloader (the classloader `==` check in the lookup for-loop).
But I agree with you that for lookup purposes it's better to use the 1-to-1 relation between classloaders and deployments. That's why I additionally introduced `Map<ClassLoader, GridDeployment> depsByLdr`.
But at the same time I kept the `Map<ClassLoader, Deque<GridDeployment>> cacheByLdr` field in order to preserve the existing contract of cache, where each alias/class is mapped to a colleciton of deployments associated with the same classloader.
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836663472
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.stream.Collectors;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.junit.Test;
+
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+import static org.apache.ignite.testframework.GridTestUtils.waitForAllFutures;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final int NODE_CNT = 3;
+
+ /** */
+ private static final int CLIENT_CNT = 3;
+
+ /** */
+ protected static final int EXEC_CNT = 10;
+
+ /** */
+ private List<DeploymentListeningLogger> logs;
+
+ /** */
+ private List<IgniteClient> clients;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ DeploymentListeningLogger testLog = new DeploymentListeningLogger(log);
+ logs.add(testLog);
+
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ logs = new ArrayList<>(NODE_CNT);
+
+ clients = new ArrayList<>(CLIENT_CNT);
+
+ setLoggerDebugLevel();
+
+ startGrids(NODE_CNT);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ clients.clear();
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once per thin client (initial task execution).
+ */
+ @Test
+ public void testNoExcessiveLocalDeployment() {
+ try {
+ ClusterNode[] allServerNodes = grid(0).cluster().forServers().nodes().toArray(new ClusterNode[0]);
+
+ for (int i = 0; i < CLIENT_CNT; i++)
+ clients.add(startClient(allServerNodes));
+
+ 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]));
+
+ List<String> allNotFound = new ArrayList<>();
+
+ for (DeploymentListeningLogger log : logs)
+ allNotFound.addAll(log.depNotFound());
+
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ assertEquals(CLIENT_CNT, Collections.frequency(allNotFound, notFoundMsg));
+ }
+ finally {
+ clients.forEach(IgniteClient::close);
+ }
+ }
+
+ /** */
+ private static void executeTasksOnClient(IgniteClient client, int cnt, long timeout) {
+ for (int i = 0; i < cnt; i++) {
+ 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();
Review Comment:
Used this snippet, 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2836704073
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -296,23 +299,49 @@ private GridDeployment deploy(
try {
Deque<GridDeployment> cachedDeps = null;
- // Find existing class loader info.
- for (Deque<GridDeployment> deps : cache.values()) {
- for (GridDeployment d : deps) {
- if (d.classLoader() == ldr) {
- // Cache class and alias.
- fireEvt = d.addDeployedClass(cls, alias);
+ Deque<GridDeployment> depsByLdr = cacheByLdr.get(ldr);
Review Comment:
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`).
<img width="1203" height="852" alt="local deployments cache - 1" src="https://github.com/user-attachments/assets/d9435325-bb3b-412e-be71-59ddf391cca1" />
<img width="1175" height="862" alt="local deployments cache - 2" src="https://github.com/user-attachments/assets/0d7877ae-72cd-4540-ba02-e2705eb3d5c3" />
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r2850143659
##########
modules/core/src/test/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStoreReuseTest.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.ignite.internal.managers.deployment;
+
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.configuration.ThinClientConfiguration;
+import org.apache.ignite.internal.client.thin.AbstractThinClientTest;
+import org.apache.ignite.internal.client.thin.TestTask;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.config.Configurator;
+import org.apache.logging.log4j.core.config.LoggerConfig;
+import org.junit.Test;
+
+/** */
+public class GridDeploymentLocalStoreReuseTest extends AbstractThinClientTest {
+ /** */
+ private static final String LOG_NAME = "org.apache.ignite";
+
+ /** */
+ private static final int EXEC_CNT = 3;
+
+ /** */
+ private static Level initLogLevel;
+
+ /** */
+ private static ListeningTestLogger testLog;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ return super.getConfiguration(igniteInstanceName)
+ .setClientConnectorConfiguration(
+ new ClientConnectorConfiguration().setThinClientConfiguration(
+ new ThinClientConfiguration().setMaxActiveComputeTasksPerConnection(1000)))
+ .setGridLogger(testLog)
+ .setPeerClassLoadingEnabled(true);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ LoggerConfig logCfg = LoggerContext.getContext(false).getConfiguration().getLoggerConfig(LOG_NAME);
+
+ initLogLevel = logCfg.getLevel();
+
+ Configurator.setLevel(LOG_NAME, Level.TRACE);
+
+ testLog = new ListeningTestLogger(log);
+
+ startGrids(2);
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ stopAllGrids();
+
+ Configurator.setLevel(LOG_NAME, initLogLevel);
+
+ initLogLevel = null;
+
+ super.afterTest();
+ }
+
+ /**
+ * Verifies that multiple task executions do not cause excessive local deployment cache misses. The "deployment not
+ * found ... clsLdrId=null" message is allowed only once upon initial task execution. The trace-level "deployment
+ * was found for class with the local app class loader" messages must be present in the output marking the subsequent
+ * task executions without cache misses.
+ */
+ @Test
+ public void testNoExcessiveLocalDeploymentCacheMisses() throws Exception {
+ String taskClsName = TestTask.class.getName();
+
+ String notFoundMsg = String.format(
+ "Deployment was not found for class with specific class loader [alias=%s, clsLdrId=null]", taskClsName);
+
+ String foundLocLdrMsg = String.format(
+ "Deployment was found for class with the local app class loader [alias=%s]", taskClsName);
+
+ LogListener lsnr0 = LogListener.matches(notFoundMsg).times(1).build();
+ LogListener lsnr1 = LogListener.matches(foundLocLdrMsg).atLeast(EXEC_CNT - 1).build();
+
+ testLog.registerListener(lsnr0);
+ testLog.registerListener(lsnr1);
+
+ try (IgniteClient client = startClient(0)) {
+ for (int i = 0; i < EXEC_CNT; i++)
+ client.compute().execute(TestTask.class.getName(), null);
+ }
+
+ assertTrue(lsnr0.check(5_000));
+ assertTrue(lsnr1.check(5_000));
Review Comment:
Removed timeout.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r3030713371
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -543,14 +535,16 @@ private String getAlias(GridDeployment dep, Class<?> cls) {
* @param ldr Class loader to undeploy.
*/
private void undeploy(ClassLoader ldr) {
- Collection<GridDeployment> doomed = new HashSet<>();
+ Collection<GridDeployment> doomed = U.newIdentityHashSet();
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();) {
Review Comment:
Done as part of refactoring.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27871 Improve deployment lookup to reduce deploy() contention … [ignite]
Posted by "oleg-vlsk (via GitHub)" <gi...@apache.org>.
oleg-vlsk commented on code in PR #12760:
URL: https://github.com/apache/ignite/pull/12760#discussion_r3030712013
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java:
##########
@@ -543,14 +535,16 @@ private String getAlias(GridDeployment dep, Class<?> cls) {
* @param ldr Class loader to undeploy.
*/
private void undeploy(ClassLoader ldr) {
- Collection<GridDeployment> doomed = new HashSet<>();
+ Collection<GridDeployment> doomed = U.newIdentityHashSet();
Review Comment:
Refactored the `undeploy()` method based on the 1:1 classloder to deployment relation.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org