Skip to content

WIP DO NOT MERGE Fixes to testManuallyAdd... #604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
geomacy wants to merge 36 commits into from
Closed

WIP DO NOT MERGE Fixes to testManuallyAdd... #604

geomacy wants to merge 36 commits into from

Conversation

@geomacy
Copy link
Contributor

@geomacy geomacy commented Mar 22, 2017 •
edited
Loading

Make initial changes as suggested by Alex in #573.
To be merged after #338. Only the last (a5cdc19) of the changes in this PR is actually not part of 338.

Add addToCatalogSearchPathOnAddition and make it set search paths.
Update management context classloader provider
Add getCatalogItemIdAndSearchPathFromContext.
Set context details as search path in AbstractBrooklynObject constructor.
Update testManuallyAddWithParentFromCatalog assertion as suggested.
Update testManuallyAddInTaskOfOtherEntity assertion as suggested.

The changes above allow the tests just named to succeed, however
they have knock-on effects in other tests, as they change the
behaviour of DSL "scopeRoot", which effectively relies on
the catalogItemId of children being copied from that of their
parents. This means the following tests fail:

Failed tests:
EntitiesYamlTest.testScopeReferences:569->assertScopes:581 expected [ReferencingYamlTestEntityImpl{id=f3z5pyzt8r}] but found [ReferencingYamlTestEntityImpl{id=witrlgcuo1}]
EntityRefsYamlTest.testRefToScopeRoot:101 expected [TestEntityImpl{id=fc9nfhoj8x}] but found [TestEntityImpl{id=g2ai718knw}]
DslYamlTest.testDslScopeRoot:253->assertScopeRoot:261 expected [BasicEntityImpl{id=a860h6w3at}] but found [BasicEntityImpl{id=aoxrtylbv2}]

geomacy added 30 commits March 21, 2017 09:15
@geomacy
Osgi loader change
9e905d5
@geomacy
Add nested catalog item ids to AbstractBrooklynObjectSpec.
5918da5
@geomacy
Add catalog item super id support.
f9fe7b8
@geomacy
Fixes to get CatalogYamlEntityTest.testCatalogItemIdInReferencedItems…
ce4df3f 
… working.

Haven't re-run all unit tests yet.
@geomacy
Test with a 'catalog sandwich'.
e34468e 
The purpose of this test is to illustrate that the full stack of nested
catalog items is not searched for classloaders.
@geomacy
Do resource lookup from nested catalog ids.
eb71ef2 
Fixes testDeepCatalogItemCanLoadResources.
@geomacy
Use entity classloader as well as specified libraries.
e59a9f2 
See apache#338 (comment).
@geomacy
Implement getCatalogItemSuperIds in CatalogItemDtoAbstract.
d449455 
As noted at apache#338 (comment)
@geomacy
Add (disabled) rebind test for deep catalog item nesting.
2faf1bc 
This fails with the following exception:

org.apache.brooklyn.util.exceptions.PropagatedRuntimeException: Failure rebinding: problem creating ENTITY zmm0lmrzll of type org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: Unable to load org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl from []: Invalid class: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at org.apache.brooklyn.util.exceptions.Exceptions.propagate(Exceptions.java:129)
	at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl.rebind(RebindManagerImpl.java:513)
	at org.apache.brooklyn.core.mgmt.rebind.RebindTestUtils.rebindAll(RebindTestUtils.java:446)
	at org.apache.brooklyn.core.mgmt.rebind.RebindTestUtils.rebind(RebindTestUtils.java:328)
	at org.apache.brooklyn.core.mgmt.rebind.RebindTestFixture.rebind(RebindTestFixture.java:281)
	at org.apache.brooklyn.camp.brooklyn.AbstractYamlRebindTest.rebind(AbstractYamlRebindTest.java:86)
	at org.apache.brooklyn.core.mgmt.rebind.RebindTestFixture.rebind(RebindTestFixture.java:217)
	at org.apache.brooklyn.camp.brooklyn.RebindOsgiTest.testReboundDeepCatalogItemCanLoadResources(RebindOsgiTest.java:201)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:497)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:84)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:714)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:901)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1231)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:127)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:111)
	at org.testng.TestRunner.privateRun(TestRunner.java:767)
	at org.testng.TestRunner.run(TestRunner.java:617)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:348)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:343)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:305)
	at org.testng.SuiteRunner.run(SuiteRunner.java:254)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1224)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1149)
	at org.testng.TestNG.run(TestNG.java:1057)
	at org.testng.IDEARemoteTestNG.run(IDEARemoteTestNG.java:72)
	at org.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:124)
Caused by: java.util.concurrent.ExecutionException: org.apache.brooklyn.util.exceptions.PropagatedRuntimeException: Failure rebinding: problem creating ENTITY zmm0lmrzll of type org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: Unable to load org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl from []: Invalid class: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.util.concurrent.FutureTask.get(FutureTask.java:192)
	at com.google.common.util.concurrent.ForwardingFuture.get(ForwardingFuture.java:63)
	at org.apache.brooklyn.util.core.task.BasicTask.get(BasicTask.java:361)
	at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl.rebind(RebindManagerImpl.java:511)
	... 29 more
Caused by: org.apache.brooklyn.util.exceptions.PropagatedRuntimeException: Failure rebinding: problem creating ENTITY zmm0lmrzll of type org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: Unable to load org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl from []: Invalid class: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at org.apache.brooklyn.util.exceptions.Exceptions.create(Exceptions.java:430)
	at org.apache.brooklyn.core.mgmt.rebind.RebindExceptionHandlerImpl.onDoneImpl(RebindExceptionHandlerImpl.java:497)
	at org.apache.brooklyn.core.mgmt.rebind.RebindExceptionHandlerImpl.onDone(RebindExceptionHandlerImpl.java:413)
	at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.run(RebindIteration.java:268)
	at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl.rebindImpl(RebindManagerImpl.java:558)
	at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl$3.call(RebindManagerImpl.java:508)
	at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl$3.call(RebindManagerImpl.java:506)
	at org.apache.brooklyn.util.core.task.BasicExecutionManager$SubmissionCallable.call(BasicExecutionManager.java:519)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.IllegalStateException: problem creating ENTITY zmm0lmrzll of type org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at org.apache.brooklyn.core.mgmt.rebind.RebindExceptionHandlerImpl.onCreateFailed(RebindExceptionHandlerImpl.java:265)
	at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.instantiateLocationsAndEntities(RebindIteration.java:459)
	at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.doRun(RebindIteration.java:240)
	at org.apache.brooklyn.core.mgmt.rebind.InitialFullRebindIteration.doRun(InitialFullRebindIteration.java:69)
	at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.run(RebindIteration.java:266)
	... 8 more
Caused by: java.lang.IllegalStateException: Unable to load org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl from []: Invalid class: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at org.apache.brooklyn.util.guava.IllegalStateExceptionSupplier.get(IllegalStateExceptionSupplier.java:40)
	at org.apache.brooklyn.util.guava.IllegalStateExceptionSupplier.get(IllegalStateExceptionSupplier.java:26)
	at org.apache.brooklyn.util.guava.Maybe$Absent.getException(Maybe.java:327)
	at org.apache.brooklyn.util.guava.Maybe$Absent.get(Maybe.java:321)
	at org.apache.brooklyn.core.mgmt.classloading.AbstractBrooklynClassLoadingContext.loadClass(AbstractBrooklynClassLoadingContext.java:55)
	at org.apache.brooklyn.core.mgmt.rebind.RebindIteration$BrooklynObjectInstantiator.load(RebindIteration.java:954)
	at org.apache.brooklyn.core.mgmt.rebind.RebindIteration$BrooklynObjectInstantiator.newEntity(RebindIteration.java:874)
	at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.instantiateLocationsAndEntities(RebindIteration.java:454)
	... 11 more
Caused by: org.apache.brooklyn.util.exceptions.PropagatedRuntimeException: Unable to load org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl from []: Invalid class: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl: ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at org.apache.brooklyn.util.exceptions.Exceptions.create(Exceptions.java:430)
	at org.apache.brooklyn.core.mgmt.classloading.BrooklynClassLoadingContextSequential.tryLoadClass(BrooklynClassLoadingContextSequential.java:88)
	at org.apache.brooklyn.core.mgmt.classloading.AbstractBrooklynClassLoadingContext.tryLoadClass(AbstractBrooklynClassLoadingContext.java:61)
	... 15 more
Caused by: java.lang.IllegalStateException: Invalid class: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at org.apache.brooklyn.util.guava.IllegalStateExceptionSupplier.get(IllegalStateExceptionSupplier.java:40)
	at org.apache.brooklyn.util.guava.IllegalStateExceptionSupplier.get(IllegalStateExceptionSupplier.java:26)
	at org.apache.brooklyn.util.guava.Maybe$Absent.getException(Maybe.java:327)
	at org.apache.brooklyn.core.mgmt.classloading.BrooklynClassLoadingContextSequential.tryLoadClass(BrooklynClassLoadingContextSequential.java:85)
	... 16 more
Caused by: java.lang.ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.SimpleEntityImpl
	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at org.apache.brooklyn.util.javalang.AggregateClassLoader.findClass(AggregateClassLoader.java:135)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at org.apache.brooklyn.core.mgmt.classloading.JavaBrooklynClassLoadingContext.tryLoadClass0(JavaBrooklynClassLoadingContext.java:101)
	at org.apache.brooklyn.core.mgmt.classloading.JavaBrooklynClassLoadingContext.tryLoadClass(JavaBrooklynClassLoadingContext.java:84)
	at org.apache.brooklyn.core.mgmt.classloading.BrooklynClassLoadingContextSequential.tryLoadClass(BrooklynClassLoadingContextSequential.java:81)
	... 16 more
@geomacy
Add support for catalogItemSuperIds to rebind.
d1fdccd 
Fixes testReboundDeepCatalogItemCanLoadResources.
@geomacy
Fix ActivePartialRebindVersionTest breakage from previous change.
66efeb5
@geomacy
Fix testRebindWithCatalogAndApp with mode == DELETE_CATALOG.
6ece325
@geomacy
Fix testRebindWithCatalogAndApp with mode == REPLACE_CATALOG_WITH_NEW…
f5360af 
…ER_VERSION.
@geomacy
Add (some) tests of rebind of previously persisted state.
97831b8
@geomacy
Some code tidy-up.
2c52500
@geomacy
Warning on primaries not needed.
00be1fc 
It's perfectly possible for the catalog item loader to have just the launcher management context in the secondaries.
@geomacy
catalogItemIdIfNotNull is no longer used
738435f
@geomacy
Add start of a test framework test for the catalog behaviour.
16c9f9e
@geomacy
Fix mistake in merge with master
e414b39
@geomacy
Fix tests after rebase with master
851ce25
@geomacy
Updates for the following review comments.
d0cc8e0 
Working on review comments for catalog-update PR

apache#338 (comment)
Unused import in AbstractBrooklynObjectSpec.java  - done in merge

apache#338 (comment)
Keep for persistence backwards compatibility
- surely I take this into account already? See testRebindWithCatalogAndAppRebindCatalogItemIds,
and BrooklynMementoPersisterToObjectStore.getCatalogItemIds().

apache#338 (comment)
Not called on xstream deserialize, could be null - take into account in following code.
- Introduced getCatalogItemIdStack().  What about the other pre-existing fields that also have initializers?

apache#338 (comment)
  public SpecT catalogItemId(String val) {
Should we deprecate this one? When is this one used vs nestCatalogItemId?
I'd think (before looking at the code in details) that this shouldn't be called at all, we should only be "nesting" catalog items.
- deprecated

https://github.com/apache/brooklyn-server/pull/338/files#r94615424
Don't see this used, do you think we should keep it?
- replaced it with protected SpecT catalogItemIdStack(Collection<String> catalogItemIdStack),
used in AbstractBrooklynObjectSpec#copyFrom().

https://github.com/apache/brooklyn-server/pull/338/files#r94609346
missing word
- fixed

https://github.com/apache/brooklyn-server/pull/338/files#r94613348
It's not a mere reference but R3 extending R2, etc.
- fixed

apache#338 (comment)
public SpecT catalogItemIdIfNotNull(String val) {
I'd deprecate it, even if marked as @beta.
- done

https://github.com/apache/brooklyn-server/pull/338/files#r94616318
I don't associate "nest" with anything in this context, perhaps stackCatalogItemId would be more appropriate?
- changed to "stackCatalogItemId".

apache#338 (comment)
public final String getCatalogItemId() {
Suggest deprecating this one and creating getOuterCatalogItemId, getInnerCatalogItemId.
- created getOuterCatalogItemId; getInner not needed as there is getCatalogItemHierarchy

apache#338 (comment)
As a fallback return catalogItemId.
- Seems preferable not to retain catalogItemId if we can get away without it, see
comment on apache#338 (comment)
above.

apache#338 (comment)
Can you add as a clarification
- Added.

apache#338 (comment)
"Super" (here and same name in other classes) could be misinterpreted
- changed name to getCatalogItemHierarchy
@geomacy
Updates for the following review comments.
55a2568 
apache#338 (comment)
This is just for persistence backwards compatibility, right? If so mark it as deprecated.
- done

apache#338 (comment)
Unused.
- done

apache#338 (comment)
Unused, plus Strings, XmlUtil.
- done

apache#338 (comment)
At the very least could log at higher level - code at AbstractManagementContext used to log at error.
- changed to warn.

apache#338 (comment)
Use and return getCatalogItemSuperIds instead.
- updated
@geomacy
Fall through only if loader fails with the available catalog items.
dc0eaa8 
apache#338 (comment)
@geomacy
Address the following review comments.
934ae5f 
apache#338 (comment)
Remove/deprecate this method and use getCatalogItemSuperIds instead in callers.
- done; also renames "SuperIds" to "Hierarchy" everywhere.
@geomacy
Review comment. Remove confusing default value.
9e7950b 
apache#338 (comment)
@geomacy
Delete original change from 0ab9b37
22c36a6 
apache#338 (comment)

The change was done during the early investigation of the issue.
The nested-item tests work without it.
@geomacy
Use newClassLoadingContextForCatalogItems where possible.
59f0cba 
https://github.com/apache/brooklyn-server/pull/338/files#r94623208

Use in

org.apache.brooklyn.util.core.ClassLoaderUtils#load
org.apache.brooklyn.core.mgmt.persist.BrooklynMementoPersisterToObjectStore#getCustomClassLoaderForBrooklynObject
org.apache.brooklyn.core.catalog.internal.JavaCatalogToSpecTransformer#createCatalogSpec

Not doing XmlMementoSerializer as it is calling
  newClassLoadingContext(ManagementContext mgmt, RegisteredType item)
rather than
  newClassLoadingContext(ManagementContext mgmt, CatalogItem<?, ?> item),
and the hierarchy is not available on RegisteredType.
@geomacy
Return a generic context from newClassLoadingContextForCatalogItems
8a1255c 
apache#338 (comment)
@geomacy
Changes to move to catalogItemId + search path.
d61aff7 
Restore catalogItemId; change getCatalogItemIdHierarchy to getCatalogItemIdSearchPath.
Update "stackCatalogItemId" to work with catalogItemId + search path.
Use catalogItemIdAndSearchPath when merging wrapper parent to child.
Rebind changes for  catalogItemId + search path.
Update AbstractBrooklynObject[Spec] for catalogItemId + searchPath
Do catalogItemIdAndSearchPath only if non-null
Update "catalogItemIdHierarchy" -> "searchPath" in persistence
@geomacy
Correct testRebindWithCatalogAndAppRebindCatalogItemIds
86d7a57
geomacy added 6 commits March 21, 2017 09:37
@geomacy
XPathHelper.getStringList fix and formatting, hashcode/equals fixes.
4e883cf
@geomacy
Add searchPath to Entities.dumpInfo.
0383e10
@geomacy
Set searchPath in JavaCatalogToSpecTransformer.
2ce534a
@geomacy
Update comments on catalogItemId().
8f37ac5
@geomacy
Do setCatalogItemIdAndSearchPath instead of just set search path.
d351bd5
@geomacy
Make initial changes as suggested by Alex in apache#573.
a5cdc19 
Add addToCatalogSearchPathOnAddition and make it set search paths.
Update management context classloader provider
Add getCatalogItemIdAndSearchPathFromContext.
Set context details as search path in AbstractBrooklynObject constructor.
Update testManuallyAddWithParentFromCatalog assertion as suggested.
Update testManuallyAddInTaskOfOtherEntity assertion as suggested.

The changes above allow the tests just named to succeed, however
they have knock-on effects in other tests, as they change the
behaviour of DSL "scopeRoot", which effectively relies on
the catalogItemId of children being copied from that of their
parents. This means the following tests fail:

Failed tests:
  EntitiesYamlTest.testScopeReferences:569->assertScopes:581 expected [ReferencingYamlTestEntityImpl{id=f3z5pyzt8r}] but found [ReferencingYamlTestEntityImpl{id=witrlgcuo1}]
  EntityRefsYamlTest.testRefToScopeRoot:101 expected [TestEntityImpl{id=fc9nfhoj8x}] but found [TestEntityImpl{id=g2ai718knw}]
  DslYamlTest.testDslScopeRoot:253->assertScopeRoot:261 expected [BasicEntityImpl{id=a860h6w3at}] but found [BasicEntityImpl{id=aoxrtylbv2}]
@geomacy geomacy mentioned this pull request Mar 22, 2017
Merged
@geomacy geomacy changed the title Catalog update continued WIP DO NOT MERGE Fixes to testManuallyAdd... Mar 23, 2017
@geomacy geomacy mentioned this pull request Apr 24, 2017
Merged
@geomacy
Copy link
Contributor Author

geomacy commented Jun 6, 2017

Just going to close this PR, it can be revived when we think it's important to get round to those comments.

@geomacy geomacy closed this Jun 6, 2017
@geomacy geomacy deleted the catalog-update-continued branch June 12, 2017 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@geomacy