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 2017/03/16 00:22:53 UTC
[GitHub] brooklyn-server pull request #598: Fix rebind for classrename with bundle pr...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/598
Fix rebind for classrename with bundle prefix
The historic persisted state added here for testing comes from a clocker.io blueprint.
The "Fix classRenames for SshPollConfig anonymous classes" is me playing it safe, to re-add the anonymous inner classes that might be referenced by customers out there. As the comment in the code says, we can't just use `deserializingClassRenames.properties`, because persistence format for a static versus anonymous (non-static) inner class is different.
For the "Fix xstream deserialising osgi class renames", I discussed this with @neykov first. We both agreed that we don't particularly like the fix, but that at least it's localised and it fixes the problem (which is causing rebind to older versions of Brooklyn to fail). We'd like to revisit this, and see about pushing the calls to the class-renames into `ClassLoaderUtils` perhaps. But that's a separate discussion, not for this PR.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server fix-rebind-classrename-failure
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/598.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 #598
----
commit ad73fcf99676b4c06e30ac6f1ed1093225087f05
Author: Aled Sage <al...@gmail.com>
Date: 2017-03-16T00:04:03Z
Fix classRenames for SshPollConfig anonymous classes
commit bcaf76bc8e5825c765d6511541ca5edb3372d6b0
Author: Aled Sage <al...@gmail.com>
Date: 2017-03-15T19:24:24Z
Testing rebind of SshFeed (historic state)
commit 1482bf0cfc725478ad48ffc101896e62aeee2bfc
Author: Aled Sage <al...@gmail.com>
Date: 2017-03-15T19:20:43Z
Fix xstream deserialising osgi class renames
i.e. where the class name in the persisted state includes the
bundle symbolic name as a prefix.
----
---
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] brooklyn-server pull request #598: BROOKLYN-453: Fix rebind for classrename ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/598
---
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] brooklyn-server issue #598: BROOKLYN-453: Fix rebind for classrename with bu...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/598
jenkins test failure looked unrelated - we should investigate that separately. I've amended the commits to force jenkins to build again.
For the record, here's the test failure:
```
2017-03-16 00:39:32,545 INFO TESTNG INVOKING: "Surefire test" - org.apache.brooklyn.entity.software.base.SoftwareProcessStopsDuringStartTest.testStopDuringProvisionWaitsForCompletion()
2017-03-16 00:39:32,549 INFO Stopping EmptySoftwareProcessImpl{id=ax1j5mlygp} in []
2017-03-16 00:39:32,550 INFO When stopping EmptySoftwareProcessImpl{id=ax1j5mlygp}, waiting for up to 10m for the machine to finish provisioning, before terminating it
2017-03-16 00:39:32,554 WARN Deprecated use of unmanaged location (SshMachineLocation[SshMachineLocation:y2op:null@localhost/127.0.0.1:22(id=y2ope2zmax)]); will be managed automatically now but not supported in future versions
2017-03-16 00:39:32,555 INFO Simulated obtain of machine SshMachineLocation[SshMachineLocation:y2op:null@localhost/127.0.0.1:22(id=y2ope2zmax)]
2017-03-16 00:39:32,574 WARN Request for bundle 'org.apache.brooklyn.software-winrm' (0.11.0-SNAPSHOT) will be ignored, loading 'org.apache.brooklyn.location.winrm.WinRmMachineLocation' as no framework available
2017-03-16 00:39:32,730 INFO TESTNG FAILED: "Surefire test" - org.apache.brooklyn.entity.software.base.SoftwareProcessStopsDuringStartTest.testStopDuringProvisionWaitsForCompletion() finished in 183 ms
java.lang.AssertionError: lists don't have the same size expected [2] but found [1]
at org.apache.brooklyn.entity.software.base.SoftwareProcessStopsDuringStartTest.testStopDuringProvisionWaitsForCompletion(SoftwareProcessStopsDuringStartTest.java:185)
```
---
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] brooklyn-server pull request #598: BROOKLYN-453: Fix rebind for classrename ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/598#discussion_r106369844
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java ---
@@ -22,32 +22,168 @@
import java.util.Map;
+import org.apache.brooklyn.core.mgmt.persist.OsgiClassPrefixer;
import org.apache.brooklyn.util.guava.Maybe;
import org.apache.brooklyn.util.javalang.Reflections;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.base.Supplier;
+import com.thoughtworks.xstream.mapper.CannotResolveClassException;
import com.thoughtworks.xstream.mapper.Mapper;
import com.thoughtworks.xstream.mapper.MapperWrapper;
+/**
+ * An xstream mapper that handles class-renames, so we can rebind to historic persisted state.
+ */
public class ClassRenamingMapper extends MapperWrapper {
+
+ /*
+ * TODO There is a strange relationship between this and XmlMementoSerializer$OsgiClassnameMapper.
+ * Should these be perhaps merged?
+ *
+ * TODO For class-loading on deserialzation, should we push the class-rename logic into
+ * org.apache.brooklyn.util.core.ClassLoaderUtils instead? Does the xstream mapper do
+ * anything else important, beyond that class-loading responsibility? It's registration
+ * in XmlSerializer makes it look a bit scary: wrapMapperForAllLowLevelMentions().
+ *
+ * ---
+ * TODO This code feels overly complicated, and deserves a cleanup.
+ *
+ * The aim is to handle two use-cases in the deserializingClassRenames.properties:
+ *
+ * 1. A very explicit rename that includes bundle prefixes (e.g. so as to limit scope, or to support
+ * moving a class from one bundle to another).
+ *
+ * 2. Just the class-rename (e.g. `com.acme.Foo: com.acme.Bar`).
+ * This would rename "acme-bundle:com.acme.Foo" to "acme-bundle:com.acme.Bar".
+ *
+ * However, to achieve that is fiddly for several reasons:
+ *
+ * 1. We might be passed qualified or unqualified names (e.g. "com.acme.Foo" or "acme-bundle:com.acme.Foo"),
+ * depending how old the persisted state is, where OSGi was used previously, and whether
+ * whitelabelled bundles were used.
+ *
+ * 2. Calling `super.realClass(name)` must return a class that has exactly the same name as
+ * was passed in. This is because xstream will subsequently use `Class.forName` which is
+ * fussy about that. However, if we're passed "acme-bundle:com.acme.Foo" then we'd expect
+ * to return a class named "com.acme.Foo". The final classloading in our
+ * `XmlMementoSerializer$OsgiClassLoader.findClass()` will handle stripping out the bundle
+ * name, and using the right bundle.
+ *
+ * In the case where we haven't changed the name, then we can leave it up to
+ * `XmlMementoSerializer$OsgiClassnameMapper.realClass()` to do sort this out. But if we've
+ * done a rename, then unforutnately it's currently this class' responsibility!
+ *
+ * That means it has to fallback to calling classLoader.loadClass().
+ *
+ * 3. As mentioned under the use-cases, the rename could include the full bundle name prefix,
+ * or it might just be the classname. We want to handle both, so need to implement yet
+ * more fallback behaviour.
+ *
+ * ---
+ * TODO Wanted to pass xstream, rather than Supplier<ClassLoader>, in constructor. However,
+ * this caused NPE because of how this is constructed from inside
+ * XmlMementoSerializer.wrapMapperForNormalUsage, called from within an anonymous subclass of XStream!
+ */
+
public static final Logger LOG = LoggerFactory.getLogger(ClassRenamingMapper.class);
private final Map<String, String> nameToType;
-
- public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType) {
+ private final Supplier<? extends ClassLoader> classLoaderSupplier;
+
+ public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType, Supplier<? extends ClassLoader> classLoaderSupplier) {
super(wrapped);
this.nameToType = checkNotNull(nameToType, "nameToType");
+ this.classLoaderSupplier = checkNotNull(classLoaderSupplier, "classLoaderSupplier");
}
@Override
public Class<?> realClass(String elementName) {
+ String elementNamOrig = elementName;
--- End diff --
Typo elementNam**e**Orig
---
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.
---