You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/02/08 18:40:55 UTC
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8759: IGNITE-14131 Fix problems with concurrent ignite.compute call
vldpyatkov commented on a change in pull request #8759:
URL: https://github.com/apache/ignite/pull/8759#discussion_r572242933
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java
##########
@@ -232,23 +230,33 @@
/** {@inheritDoc} */
@Override public GridDeployment searchDeploymentCache(GridDeploymentMetadata meta) {
- return deployment(meta.alias());
+ return deployment(meta);
}
/**
- * @param alias Class alias.
+ * @param meta Deployment meta.
* @return Deployment.
*/
- @Nullable private GridDeployment deployment(String alias) {
- Deque<GridDeployment> deps = cache.get(alias);
+ @Nullable private GridDeployment deployment(final GridDeploymentMetadata meta) {
+ Deque<GridDeployment> deps = cache.get(meta.alias());
if (deps != null) {
- GridDeployment dep = deps.peekFirst();
+ for (GridDeployment dep : deps) {
+ // local or remote deployment.
Review comment:
remote or local
##########
File path: modules/core/src/main/java/org/apache/ignite/spi/deployment/local/LocalDeploymentSpi.java
##########
@@ -111,35 +110,60 @@
}
/** {@inheritDoc} */
- @Nullable @Override public DeploymentResource findResource(String rsrcName) {
+ @Nullable @Override public DeploymentResource findResource(String rsrcName, @Nullable ClassLoader clsLdr) {
assert rsrcName != null;
- // Last updated class loader has highest priority in search.
- for (Entry<ClassLoader, ConcurrentMap<String, String>> e : ldrRsrcs.descendingEntrySet()) {
+ if (clsLdr != null) {
+ ConcurrentMap<String, String> rsrcs = ldrRsrcs.get(clsLdr);
+
+ if (rsrcs == null)
+ return null;
+
+ return findResource0(rsrcs, rsrcName, clsLdr);
+ }
+
+ for (Entry<ClassLoader, ConcurrentMap<String, String>> e : ldrRsrcs.entrySet()) {
Review comment:
It is a very suspected way, because you cannot be sure what the specific class loader will be chosen.
Is it only for deprecated functionality?
If yest, please extract it to another method and mark it @Deperecated.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentLocalStore.java
##########
@@ -109,12 +109,10 @@
@Override public Collection<GridDeployment> getDeployments() {
Collection<GridDeployment> deps = new ArrayList<>();
- synchronized (mux) {
Review comment:
How do you can get rid of mux here?
##########
File path: modules/core/src/main/java/org/apache/ignite/spi/deployment/DeploymentSpi.java
##########
@@ -63,9 +63,10 @@
* Finds class loader for the given class.
*
* @param rsrcName Class name or class alias to find class loader for.
+ * @param clsLdr desired class loader.
* @return Deployed class loader, or {@code null} if not deployed.
*/
- public DeploymentResource findResource(String rsrcName);
Review comment:
You cannot remove a method from the public API.
Just mark it as deprecated.
##########
File path: modules/core/src/main/java/org/apache/ignite/spi/deployment/local/LocalDeploymentSpi.java
##########
@@ -111,35 +110,60 @@
}
/** {@inheritDoc} */
- @Nullable @Override public DeploymentResource findResource(String rsrcName) {
+ @Nullable @Override public DeploymentResource findResource(String rsrcName, @Nullable ClassLoader clsLdr) {
assert rsrcName != null;
- // Last updated class loader has highest priority in search.
- for (Entry<ClassLoader, ConcurrentMap<String, String>> e : ldrRsrcs.descendingEntrySet()) {
+ if (clsLdr != null) {
+ ConcurrentMap<String, String> rsrcs = ldrRsrcs.get(clsLdr);
+
+ if (rsrcs == null)
+ return null;
+
+ return findResource0(rsrcs, rsrcName, clsLdr);
+ }
+
+ for (Entry<ClassLoader, ConcurrentMap<String, String>> e : ldrRsrcs.entrySet()) {
ClassLoader ldr = e.getKey();
ConcurrentMap<String, String> rsrcs = e.getValue();
- String clsName = rsrcs.get(rsrcName);
+ DeploymentResourceAdapter res = findResource0(rsrcs, rsrcName, ldr);
- // Return class if it was found in resources map.
- if (clsName != null) {
- // Recalculate resource name in case if access is performed by
- // class name and not the resource name.
- rsrcName = getResourceName(clsName, rsrcs);
+ if (res != null)
+ return res;
+ }
- assert clsName != null;
+ return null;
+ }
- try {
- Class<?> cls = Class.forName(clsName, true, ldr);
+ /**
+ * Finds appropriate resource.
+ *
+ * @param rsrcs Resources.
+ * @param rsrcName Class name or class alias to find class loader for.
+ * @param clsLdr desired class loader.
+ * @return Deployed class loader, or {@code null} if not deployed.
+ */
+ @Nullable private DeploymentResourceAdapter findResource0(Map<String, String> rsrcs, String rsrcName, ClassLoader clsLdr) {
+ String clsName = rsrcs.get(rsrcName);
- assert cls != null;
+ // Return class if it was found in resources map.
+ if (clsName != null) {
+ // Recalculate resource name in case if access is performed by
+ // class name and not the resource name.
+ rsrcName = getResourceName(clsName, rsrcs);
- // Return resource.
- return new DeploymentResourceAdapter(rsrcName, cls, ldr);
- }
- catch (ClassNotFoundException ignored) {
- // No-op.
- }
+ assert clsName != null;
+
+ try {
+ Class<?> cls = Class.forName(clsName, true, clsLdr);
+
+ assert cls != null;
+
+ // Return resource.
+ return new DeploymentResourceAdapter(rsrcName, cls, clsLdr);
+ }
+ catch (ClassNotFoundException ignored) {
Review comment:
Why do you ignore this exception?
##########
File path: modules/core/src/main/java/org/apache/ignite/spi/deployment/local/LocalDeploymentSpi.java
##########
@@ -111,35 +110,60 @@
}
/** {@inheritDoc} */
- @Nullable @Override public DeploymentResource findResource(String rsrcName) {
+ @Nullable @Override public DeploymentResource findResource(String rsrcName, @Nullable ClassLoader clsLdr) {
assert rsrcName != null;
- // Last updated class loader has highest priority in search.
- for (Entry<ClassLoader, ConcurrentMap<String, String>> e : ldrRsrcs.descendingEntrySet()) {
+ if (clsLdr != null) {
+ ConcurrentMap<String, String> rsrcs = ldrRsrcs.get(clsLdr);
+
+ if (rsrcs == null)
+ return null;
+
+ return findResource0(rsrcs, rsrcName, clsLdr);
+ }
+
+ for (Entry<ClassLoader, ConcurrentMap<String, String>> e : ldrRsrcs.entrySet()) {
ClassLoader ldr = e.getKey();
ConcurrentMap<String, String> rsrcs = e.getValue();
- String clsName = rsrcs.get(rsrcName);
+ DeploymentResourceAdapter res = findResource0(rsrcs, rsrcName, ldr);
- // Return class if it was found in resources map.
- if (clsName != null) {
- // Recalculate resource name in case if access is performed by
- // class name and not the resource name.
- rsrcName = getResourceName(clsName, rsrcs);
+ if (res != null)
+ return res;
+ }
- assert clsName != null;
+ return null;
+ }
- try {
- Class<?> cls = Class.forName(clsName, true, ldr);
+ /**
+ * Finds appropriate resource.
+ *
+ * @param rsrcs Resources.
+ * @param rsrcName Class name or class alias to find class loader for.
+ * @param clsLdr desired class loader.
+ * @return Deployed class loader, or {@code null} if not deployed.
+ */
+ @Nullable private DeploymentResourceAdapter findResource0(Map<String, String> rsrcs, String rsrcName, ClassLoader clsLdr) {
+ String clsName = rsrcs.get(rsrcName);
- assert cls != null;
+ // Return class if it was found in resources map.
+ if (clsName != null) {
+ // Recalculate resource name in case if access is performed by
+ // class name and not the resource name.
+ rsrcName = getResourceName(clsName, rsrcs);
- // Return resource.
- return new DeploymentResourceAdapter(rsrcName, cls, ldr);
- }
- catch (ClassNotFoundException ignored) {
- // No-op.
- }
+ assert clsName != null;
+
+ try {
+ Class<?> cls = Class.forName(clsName, true, clsLdr);
Review comment:
Do you know about U.forName() method?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org