You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2015/06/08 12:52:33 UTC
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/683
BROOKLYN-149: Partial fix for rebind when catalog missing
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn fix/rebind-when-catalog-missing
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/683.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #683
----
commit 52478ec9f7c056dc869f3f6b3fe3d499858a280c
Author: Aled Sage <al...@gmail.com>
Date: 2015-06-04T21:42:55Z
Partial fix for BROOKLYN-149 (rebind when catalog deleted)
- If entities etc still on classpath, then will continue with rebind.
But if entities cannot be created, will still fail terminally.
commit 1c689909ff7ea92a218dbb03e9721e99507c54b9
Author: Aled Sage <al...@gmail.com>
Date: 2015-06-04T21:43:41Z
Tidy warnings in RebindExceptionHandlerImpl
- Previous warning (“returning null”) didn’t make sense when read in a
log, out of the context of the individual method.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r31924382
--- Diff: core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java ---
@@ -174,11 +176,23 @@ public BrooklynMementoPersisterToObjectStore(PersistenceObjectStore objectStore,
if (item==null || item.getCatalogItemId()==null) {
return null;
}
- CatalogItem<?, ?> catalogItem = CatalogUtils.getCatalogItemOptionalVersion(lookupContext.lookupManagementContext(), item.getCatalogItemId());
+ // See RebindIteration.BrooklynObjectInstantiator.load(), for handling where catalog item is missing;
+ // similar logic here.
+ String catalogItemId = item.getCatalogItemId();
+ CatalogItem<?, ?> catalogItem = CatalogUtils.getCatalogItemOptionalVersion(lookupContext.lookupManagementContext(), catalogItemId);
+ if (catalogItem == null && BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
--- End diff --
Same `if` structure comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r32022058
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -899,48 +914,64 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
}
}
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null) {
+ if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_AUTO_FIX_CATALOG_REF_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
+
+ if (catalogItem != null) {
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will auto-upgrade to "+catalogItem.getCatalogItemId());
+ catalogItemId = catalogItem.getCatalogItemId();
+ }
}
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return new LoadedClass<T>(loader.loadClass(jType, bType), catalogItemId);
} else {
- throw new IllegalStateException("No catalogItemId specified and can't load class from classpath");
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will try default class loader");
}
-
}
- }
+
+ try {
+ return new LoadedClass<T>((Class<T>)reflections.loadClass(jType), catalogItemId);
--- End diff --
Reason I didn't return null was that it would override the `catalogItemId` to be persisted, so it would then say null. I didn't want to lose the catalogItemId in the situation where it wasn't available. Instead, we'd leave that itemId in and fall back to default class loader.
I wondered about guarding it behind the feature enablement. However, neither of the (renamed) feature options seem quite right:
* `FEATURE_BACKWARDS_COMPATIBILITY_INFER_CATALOG_ITEM_ON_REBIND` is about backwards compatibility, where the `catalogItemId` was missing from old things.
* `FEATURE_AUTO_FIX_CATALOG_REF_ON_REBIND` is for updating the ref (I think that deleting the ref seems extreme there).
We should revisit this in follow-up pull requests. I'd like us to start up such that errors are shown, and can be fixed, rather than aborting entirely or just logging and ignoring.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r31924327
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -902,45 +902,53 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
- }
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null && BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return loader.loadClass(jType, bType);
} else {
- throw new IllegalStateException("No catalogItemId specified and can't load class from classpath");
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will try default class loader");
}
-
}
- }
+
+ try {
+ return (Class<T>)reflections.loadClass(jType);
+ } catch (Exception e) {
+ Exceptions.propagateIfFatal(e);
+ LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
+ }
- protected BrooklynClassLoadingContext getLoadingContextFromCatalogItemId(String catalogItemId, ClassLoader classLoader, RebindContext rebindContext) {
- Preconditions.checkNotNull(catalogItemId, "catalogItemId required (should not be null)");
- CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
- if (catalogItem != null) {
- return CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ if (catalogItemId != null) {
+ throw new IllegalStateException("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId+", or load class from classpath");
--- End diff --
Is this definitely right, an exception if the item is *not* `null`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r31923691
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -902,45 +902,53 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
- }
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null && BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
--- End diff --
Perhaps change this `if` condition to simply the `null` check and then have another `if` that switches on whether `FEATURE_INFER_CATALOG_ITEM_ON_REBIND` is true. It's easier to follow the logic this way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/683
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#issuecomment-110397615
Good to merge. Solves a frequent pain point around rebinds.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#issuecomment-110409234
Thanks @neykov - merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r31956212
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -902,45 +902,53 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
- }
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null && BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return loader.loadClass(jType, bType);
} else {
- throw new IllegalStateException("No catalogItemId specified and can't load class from classpath");
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will try default class loader");
}
-
}
- }
+
+ try {
+ return (Class<T>)reflections.loadClass(jType);
+ } catch (Exception e) {
+ Exceptions.propagateIfFatal(e);
+ LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
+ }
- protected BrooklynClassLoadingContext getLoadingContextFromCatalogItemId(String catalogItemId, ClassLoader classLoader, RebindContext rebindContext) {
- Preconditions.checkNotNull(catalogItemId, "catalogItemId required (should not be null)");
- CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
- if (catalogItem != null) {
- return CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ if (catalogItemId != null) {
+ throw new IllegalStateException("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId+", or load class from classpath");
--- End diff --
Yes, full context of method is that we've tried all available ways of loading (there are earlier return statements in the method), so will now fail. However, if no catalogItemId was specified and if the backards_compatibility feature was enabled then we'll try to infer a catalog item id.
Not particularly nice logic, having multiple return statements, but is possibly the simplest given the unfortunate number of if-else statements to deal with the different scenarios.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#issuecomment-110132479
@neykov I've incorporated the comments; can you review again please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r32023110
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -899,48 +914,64 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
}
}
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null) {
+ if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_AUTO_FIX_CATALOG_REF_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
+
+ if (catalogItem != null) {
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will auto-upgrade to "+catalogItem.getCatalogItemId());
+ catalogItemId = catalogItem.getCatalogItemId();
+ }
}
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return new LoadedClass<T>(loader.loadClass(jType, bType), catalogItemId);
} else {
- throw new IllegalStateException("No catalogItemId specified and can't load class from classpath");
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will try default class loader");
}
-
}
- }
+
+ try {
+ return new LoadedClass<T>((Class<T>)reflections.loadClass(jType), catalogItemId);
--- End diff --
This is possible even now if you use `exceptionHandler` to log the errors. Depending on the startup options the process will abort or show a warning in the web console.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r31923738
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -902,45 +902,53 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
- }
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null && BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return loader.loadClass(jType, bType);
} else {
--- End diff --
This would get moved up (see above).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r31956339
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -902,45 +902,53 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
- }
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null && BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return loader.loadClass(jType, bType);
} else {
--- End diff --
Can't move this up; we need to do the check after going into the conditional case of trying to infer the different catalogItem.
Really don't like the code, but hard (and probably not worth a lot of refactoring) to avoid all the if-else conditions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#issuecomment-109994827
Finished review. Patching the `catalogItemId` during load instead of inferring it each time it's needed will make for a more robust failover.
All of the instances where a catalog item is missing during rebind I have seen so far have been due to user error - someone deleting the item without realising it's still in use and needed by the entities on next rebind. A more general fix would be to forbid this case altogether. It will save a lot of head scratching.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r32008631
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -899,48 +914,64 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
}
}
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null) {
+ if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_AUTO_FIX_CATALOG_REF_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
+
+ if (catalogItem != null) {
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will auto-upgrade to "+catalogItem.getCatalogItemId());
+ catalogItemId = catalogItem.getCatalogItemId();
+ }
}
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return new LoadedClass<T>(loader.loadClass(jType, bType), catalogItemId);
} else {
- throw new IllegalStateException("No catalogItemId specified and can't load class from classpath");
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will try default class loader");
}
-
}
- }
+
+ try {
+ return new LoadedClass<T>((Class<T>)reflections.loadClass(jType), catalogItemId);
--- End diff --
Shouldn't this return `null` for `catalogItemId`? Why treat it differently from the upgraded-catalog-item case where the id is patched?
Could put the block behind `FEATURE_INFER_CATALOG_ITEM_ON_REBIND` check as this is part of the same logic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r32011188
--- Diff: core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---
@@ -899,48 +914,64 @@ protected void setCatalogItemId(BrooklynObject item, String catalogItemId) {
}
}
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, Memento memento) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, Memento memento) {
return load(bType, memento.getType(), memento.getCatalogItemId(), memento.getId());
}
+
@SuppressWarnings("unchecked")
- protected <T extends BrooklynObject> Class<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
+ protected <T extends BrooklynObject> LoadedClass<? extends T> load(Class<T> bType, String jType, String catalogItemId, String contextSuchAsId) {
checkNotNull(jType, "Type of %s (%s) must not be null", contextSuchAsId, bType.getSimpleName());
+
if (catalogItemId != null) {
- BrooklynClassLoadingContext loader = getLoadingContextFromCatalogItemId(catalogItemId, classLoader, rebindContext);
- return loader.loadClass(jType, bType);
- } else {
- // we have previously used reflections; not sure if that's needed?
- try {
- return (Class<T>)reflections.loadClass(jType);
- } catch (Exception e) {
- LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
- }
-
- if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_INFER_CATALOG_ITEM_ON_REBIND)) {
- //Try loading from whichever catalog bundle succeeds.
- BrooklynCatalog catalog = managementContext.getCatalog();
- for (CatalogItem<?, ?> item : catalog.getCatalogItems()) {
- BrooklynClassLoadingContext catalogLoader = CatalogUtils.newClassLoadingContext(managementContext, item);
- Maybe<Class<?>> catalogClass = catalogLoader.tryLoadClass(jType);
- if (catalogClass.isPresent()) {
- return (Class<? extends T>) catalogClass.get();
+ CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
+ if (catalogItem == null) {
+ if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_AUTO_FIX_CATALOG_REF_ON_REBIND)) {
+ // See https://issues.apache.org/jira/browse/BROOKLYN-149
+ // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem).
+ // Try loading as any version.
+ if (CatalogUtils.looksLikeVersionedId(catalogItemId)) {
+ String symbolicName = CatalogUtils.getIdFromVersionedId(catalogItemId);
+ catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName);
+
+ if (catalogItem != null) {
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will auto-upgrade to "+catalogItem.getCatalogItemId());
+ catalogItemId = catalogItem.getCatalogItemId();
+ }
}
}
- throw new IllegalStateException("No catalogItemId specified and can't load class from either classpath of catalog items");
+ }
+ if (catalogItem != null) {
+ BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ return new LoadedClass<T>(loader.loadClass(jType, bType), catalogItemId);
} else {
- throw new IllegalStateException("No catalogItemId specified and can't load class from classpath");
+ LOG.warn("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId
+ +" ("+bType.getSimpleName()+"); will try default class loader");
}
-
}
- }
+
+ try {
+ return new LoadedClass<T>((Class<T>)reflections.loadClass(jType), catalogItemId);
+ } catch (Exception e) {
+ Exceptions.propagateIfFatal(e);
+ LOG.warn("Unable to load "+jType+" using reflections; will try standard context");
+ }
- protected BrooklynClassLoadingContext getLoadingContextFromCatalogItemId(String catalogItemId, ClassLoader classLoader, RebindContext rebindContext) {
- Preconditions.checkNotNull(catalogItemId, "catalogItemId required (should not be null)");
- CatalogItem<?, ?> catalogItem = rebindContext.lookup().lookupCatalogItem(catalogItemId);
- if (catalogItem != null) {
- return CatalogUtils.newClassLoadingContext(managementContext, catalogItem);
+ if (catalogItemId != null) {
+ throw new IllegalStateException("Unable to load catalog item "+catalogItemId+" for "+contextSuchAsId+", or load class from classpath");
+ } else if (BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_BACKWARDS_COMPATIBILITY_INFER_CATALOG_ITEM_ON_REBIND)) {
--- End diff --
Looking at the code in details now it's not obvious why this (old) code repeats the logic in `findCatalogItemId`. Actually the whole catalog item inference could be encapsulated there. Just thinking out loud, no need to change it in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-149: Partial fix for reb...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/683#discussion_r31911530
--- Diff: core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java ---
@@ -174,11 +176,23 @@ public BrooklynMementoPersisterToObjectStore(PersistenceObjectStore objectStore,
if (item==null || item.getCatalogItemId()==null) {
return null;
}
- CatalogItem<?, ?> catalogItem = CatalogUtils.getCatalogItemOptionalVersion(lookupContext.lookupManagementContext(), item.getCatalogItemId());
+ // See RebindIteration.BrooklynObjectInstantiator.load(), for handling where catalog item is missing;
+ // similar logic here.
--- End diff --
Changes here won't be needed if `catalogItemId` is patched above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---