You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2018/01/12 10:06:48 UTC
[GitHub] brooklyn-server pull request #932: [WIP] collect supertypes for registered t...
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/932
[WIP] collect supertypes for registered types
this populates the `supertypes` field of `RegisteredType` instances better, now including all java supertypes (and for specs only including what it's a spec for, ie `Entity`, now excluding the `EntitySpec` class).
this is WIP as i'd also like to include declared `RegisteredType` supers.
this is slightly messy on the API however. specifically consider a YAML type `b` declaring its super as a java `type: a.A`, and a YAML type `c` declaring super `type: b`. in java, the supers of `c` would then be `[ RegisteredType[b], Class[a.A] ]` (plus any supers of `a.A`). what should this look like in REST?
currently we return a list of strings for all supers so the json response in `TypeSummary` will include `supertypes: [ b:0.0.0_SNAPSHOT, a.A ]`.
other options are available to better distinguish registered types (yaml types) from java types. we could have eg `supertypesJava: [ a.A ]` and `supertypesYaml: [ b:0.0.0_SNAPSHOT ]`. or we could have objects eg `supertypes: [ { kind: registered-type, name: b:0.0.0_SNAPSHOT }, { kind: java, name: a.A } ]`.
i think it's best to leave it as is and rely on the `:` character to distinguish yaml supertypes. otherwise we're breaking the API and making the object seem more complex. but i want to flag this bit of complexity in the semantics of the json `supertypes` field.
(opening this PR now for early review of code and to see whether anyone has strong feelings on the above.)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server supertypes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/932.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 #932
----
commit 8bcf0cb062968fa5868b3a5422d5493c9820e390
Author: Alex Heneveld <al...@...>
Date: 2018-01-12T09:19:53Z
record java supertypes as part of type validation
and do validation for catalog items (so items are stored twice, once as item, once as type), and ensure deletion also deletes both
some TODOs noted about recording RT supertypes
----
---
[GitHub] brooklyn-server issue #932: collect supertypes for registered types
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/932
@aledsage right on all counts, i was sloppy with the deprecated stuff. can't wait till we can cut all the old catalog and ideally switch to YOML too.
---
[GitHub] brooklyn-server pull request #932: collect supertypes for registered types
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/932#discussion_r162093386
--- Diff: core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java ---
@@ -483,6 +494,7 @@ public SpecT apply(org.apache.brooklyn.core.plan.PlanToSpecTransformer input) {
while (item instanceof CatalogItemDo) item = ((CatalogItemDo<T,SpecT>)item).itemDto;
if (item==null) return null;
if (item instanceof CatalogItemDtoAbstract) return (CatalogItemDtoAbstract<T,SpecT>) item;
+ CatalogItem<?, ?> item2 = getCatalogItem(item.getSymbolicName(), item.getVersion());
--- End diff --
Why call `getCatalogItem` here? Did you mean to use that in the log message below? Or was it to validate that it exists?
---
[GitHub] brooklyn-server issue #932: collect supertypes for registered types
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/932
Test failure is unrelated, presumably non-deterministic, and should be investigated separately (`VanillaSoftwareProcessYamlTest.testDisableSshPolling`):
```
java.lang.AssertionError: failed succeeds-eventually, 75 attempts, 30000ms elapsed: AssertionError: actual=[starting, running]; expected=[created, starting, running]: lists don't have the same size expected [3] but found [2]
at org.apache.brooklyn.camp.brooklyn.VanillaSoftwareProcessYamlTest.assertEventsEqualEventually(VanillaSoftwareProcessYamlTest.java:261)
at org.apache.brooklyn.camp.brooklyn.VanillaSoftwareProcessYamlTest.testDisableSshPolling(VanillaSoftwareProcessYamlTest.java:173)
Caused by: java.lang.AssertionError: actual=[starting, running]; expected=[created, starting, running]: lists don't have the same size expected [3] but found [2]
at org.apache.brooklyn.camp.brooklyn.VanillaSoftwareProcessYamlTest.assertIterablesEqual(VanillaSoftwareProcessYamlTest.java:278)
at org.apache.brooklyn.camp.brooklyn.VanillaSoftwareProcessYamlTest.access$100(VanillaSoftwareProcessYamlTest.java:64)
at org.apache.brooklyn.camp.brooklyn.VanillaSoftwareProcessYamlTest.assertEventsEqualEventually(VanillaSoftwareProcessYamlTest.java:261)
at org.apache.brooklyn.camp.brooklyn.VanillaSoftwareProcessYamlTest.testDisableSshPolling(VanillaSoftwareProcessYamlTest.java:173)
```
---
[GitHub] brooklyn-server pull request #932: collect supertypes for registered types
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/932#discussion_r162090191
--- Diff: api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java ---
@@ -63,6 +63,13 @@
@Deprecated
void deleteCatalogItem(String symbolicName, String version);
+ /** @return Deletes the item with the given {@link CatalogItem#getSymbolicName()
--- End diff --
Spurious `@return`
---
[GitHub] brooklyn-server issue #932: [WIP] collect supertypes for registered types
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/932
ps - `testDisabledApplicationCatalog()` and possibly others failing here because the `RegisteredType` that is created for the disabled catalog item isn't being updated; fix to come soon
---
[GitHub] brooklyn-server pull request #932: collect supertypes for registered types
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/932#discussion_r162093169
--- Diff: core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java ---
@@ -1441,15 +1453,23 @@ private String makeAsIndentedObject(String yaml) {
@Override
public List<? extends CatalogItem<?,?>> addItems(String yaml) {
- return addItems(yaml, false);
+ return addItems(yaml, true, false);
}
+ /** @deprecated since 1.0.0 use {@link #addItems(String)} or {@link #addItems(String, boolean, boolean)} */
+ @Deprecated
@Override
public List<? extends CatalogItem<?,?>> addItems(String yaml, boolean forceUpdate) {
+ return addItems(yaml, true, false);
--- End diff --
Should this not call `addItems(yaml, true, forceUpdate)`?
---
[GitHub] brooklyn-server pull request #932: collect supertypes for registered types
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/932
---
[GitHub] brooklyn-server issue #932: collect supertypes for registered types
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/932
Removing WIP as this passes and the TODO items re including non-java registered types in list of supers is not essential and can be done as follow-on work.
This does not change the API.
---