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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
