REST API for accessing adjuncts (including highlights) #821

ahgittin · 2017-09-13T16:19:35Z

NB: builds on #810 and #818, review and merge those first

This is then just one commit (currently) which:

adds an AdjunctApi combining all the functionality of PolicyApi and PolicyConfigApi generalized across adjunct types
same for AdjunctConfigSummary
deprecates the replaced policy-specific endpoints and related classes, and also the CatalogApi /catalog endpoint (since we are deprecating; also there is some overlap at CatalogPolicyItem and PolicyConfigSummary etc)

This is mostly a drop-in replacement for the /applications/XXX/entities/XXX/policies/* endpoints, now using /applications/XXX/entities/XXX/adjuncts/* and returning other items. Note also that:

GET /v1/applications/XXX/entities/XXX/adjuncts?adjunctType=policy filters by policy (same for other types)
adjunctType is included in the returned object
the detail object if you GET /v1/applications/XXX/entities/XXX/adjuncts/XXX includes more info (config keys aka parameters, and values)
the links on the detail object include links to start and stop if and only if the adjunct supports that; it is recommended that clients use this value or at least check its presence rather than assume the declared .../start and .../stop endpoints will always be supported, as some adjuncts don't support (such as enrichers ... though we could and might change that ... so doing this check will be future-proof)

…ts-2

ahgittin · 2017-09-13T16:22:32Z

These are the API calls, for reference. The Entity Policy and Entity Policy Config resources shown at bottom are now redundant (and deprecated).

Big changes to config key construction, following on from previous deprecation

ahgittin · 2017-09-25T12:09:53Z

rebased to incorporate deprecation around the multiple config REST classes and add a tidy API for that

tbouron · 2017-09-28T11:04:23Z

@ahgittin The description says This is then just one commit (currently) which: but there is still 1000+ additions. Is this up to date and rebase on master?

ahgittin · 2017-09-28T11:42:47Z

@tbouron It's up to date. Primarily it is still just the one commit adding the Adjunct API -- it does have a lot of lines but they are mostly boilerplate (like a couple hundred just to declare the API) ... REST cruft and annotations.

However the merge of the XxxConfig deprecation had conflicts and there is a fair bit here tidying and improving that. Still pretty straightforward though.

tbouron · 2017-09-28T11:57:52Z

Cool, thanks @ahgittin. I'm reviewing it right now.

tbouron

Thanks @ahgittin for the PR. There are few minor comments but most importantly, I tested it and found 2 interesting things:

If you run in karaf mode (which is the default now) the new endpoint is not available! It's not the case for classic though.
on the swagger page, it is never mentioned that the entire Entity Policy and Entity Policy Config are deprecated.

tbouron · 2017-09-28T11:07:13Z

api/src/main/java/org/apache/brooklyn/api/sensor/Feed.java

@@ -43,7 +43,7 @@

    /** 
     * True if everything has been _started_ (or it is starting) but not stopped,
-     * even if it is suspended; see also {@link #isActive()}
+     * even if it is suspended; see also {@link #isRunning()}


Neither of isRunning or isActive methods are available in the class. Did you want to link to somewhere else?

isRunning is inherited from EntityAdjunct, so I think this javadoc link is ok.

isRunning() is avail, inherited from EntityAdjunct

tbouron · 2017-09-28T11:11:51Z

core/src/main/java/org/apache/brooklyn/core/entity/EntityInternal.java

@@ -189,7 +189,7 @@
        void remove(AttributeSensor<?> attribute);
    }

-    public interface FeedSupport {
+    public interface FeedSupport extends Iterable<Feed> {


Is it really necessary to extend from Iterable and have an iterator() method here where we can get it from the getFeed().iterator() method (which always returns a collection) ? I don't see the benefit of creating this small shortcut

Adding this gives consistency with entity's enrichers() and policies() which both return something that implements Iterable<T>. I therefore think it's a good idea.

this is to bring it in line with other AdjunctSupport classes. have made more steps in this direction in a new commit.

tbouron · 2017-09-28T11:20:05Z

core/src/main/java/org/apache/brooklyn/core/policy/Policies.java

+            return Lifecycle.CREATED;
+        }
+
+        return Lifecycle.STOPPED;


What other adjunct should it be to arrive here? Enrichers? In this case, is an enricher really STOPPED instead of CREATED?

you are right, changed

tbouron · 2017-09-28T11:21:07Z

core/src/main/java/org/apache/brooklyn/core/policy/Policies.java

+    }
+
+    @Beta
+    public static Lifecycle inferAdjunctStatus(EntityAdjunct a) {


Sounds like this method is not in a right class. Would be more meaningful inside an Adjuncts class for example (an since it's static, it is not an issue to move it)

I would also deprecate the entire Policies class as all methods are already deprecated

good spot. moved to EntityAdjuncts and deprecated this class, removing previously deprecated methods

tbouron · 2017-09-28T11:46:17Z

rest/rest-api/src/main/java/org/apache/brooklyn/rest/api/AdjunctApi.java

+            @ApiParam(value = "Entity ID or name", required = true)
+            @PathParam("entity") final String entityToken,
+            @ApiParam(value = "Filter by adjunct type", required = false)
+            @QueryParam("adjunctType") final String adjunctType);


type would be better here as we already have the context, i.e. we are looking at a specific type for an adjunct. But no strong feelings. It will also be consistent with the POST operation below

I wondered about type as well. Problem is we don't support filtering by the actual type (e.g. if someone is looking for AutoScalerPolicy only). It's a very similar discussion to the subtype/supertype/category discussion for PR https://github.com/apache/brooklyn-server/pull/810/files#diff-53a117c93b2d8af2b0b2198599c0c6d1. I think we should follow the same naming here as we do there - i.e. supertype.

no, the POST takes a specific registered type, whereas this is essentially the adjunct kind; have improved comments to make clear. would adjunctKind be a better name? or supertype for consistency with /catalog/types ?

tbouron · 2017-09-28T12:37:12Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+        EntityAdjunct adjunct = brooklyn().getAdjunct(application, entityToken, adjunctId);
+        if (adjunct instanceof Policy) ((Policy)adjunct).resume();
+        else if (adjunct instanceof Feed) ((Feed)adjunct).resume();
+        else throw WebResourceUtils.badRequest("%s does not support start/resume", adjunct);


Same as above

Do you think enrichers should support suspend/resume? If so, worth adding a TODO here.

tbouron · 2017-09-28T12:37:16Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+        EntityAdjunct adjunct = brooklyn().getAdjunct(application, entityToken, adjunctId);
+        if (adjunct instanceof Policy) ((Policy)adjunct).suspend();
+        else if (adjunct instanceof Feed) ((Feed)adjunct).suspend();
+        else throw WebResourceUtils.badRequest("%s does not support suspend", adjunct);


Same as above

tbouron · 2017-09-28T12:37:32Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+        return Response.status(Response.Status.OK).build();
+    }
+
+    public static String getStringValueForDisplay(BrooklynRestResourceUtils utils, EntityAdjunct policy, Object value) {


This method is unused, can be deleted

tbouron · 2017-09-28T12:42:49Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java

@@ -71,6 +71,8 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;

+/** @deprecated since 0.13.0 use RegisteredType methods */


Missing Javadoc link for RegisteredType

tbouron · 2017-09-28T12:48:33Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtils.java

+        }
+        for (Feed p: ((EntityInternal)entity).feeds()) {
+            if (adjunct.equals(p.getDisplayName())) return p;
+        }


You can group the loops of the same type, that would reduce in half the processing time

Depends whether we worry about a display name being the same as an id anywhere - and thus what guarantees we're trying to give.

I probably agree @tbouron. If we do want to keep them separate, we should iterate over all ids (policies, enrichers, feeds) before going on to display names.

no - in perverse case where we have feed<ID=1 name=2> and feed<ID=2 name=foo> a call to get "2" should return the latter, whereas combining loops would return the former

we do need to compare all by ID's first however

agree it's an edge case and isn't significant but think the code should stay with separate loops.

PS processing time change would not be as large as you say as the string comparison dominates execution time and that is unchanged by combining loops.

tbouron · 2017-09-28T16:45:55Z

BTW, I just realised that there is no tests coverage for the new endpoints Entity Adjuncts and Entity Adjuncts Config. Can you add that please @ahgittin ?

aledsage

LGTM - only minor comments.

aledsage · 2017-10-02T12:21:15Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+        AbstractBrooklynObjectSpec<?, ?> spec;
+        if (rt!=null) {
+            spec = brooklyn().getTypeRegistry().createSpec(rt, null, null);
+        } else {


(I see this code is taken from PolicyResource.addPolicy, and has been approved - therefore no strong feelings for this PR.)

Do we really need the fallback to java, or can we assume that it must be in the catalog? (we don't even support feeds, which aren't in the catalog yet).

Ideally I'd like us to not have to support this code path, or to have a deprecation warning logged in that else branch (telling people to add it to the catalog).

i'd like to switch to yaml for this, have added comment

aledsage · 2017-10-02T12:21:46Z

rest/rest-api/src/main/java/org/apache/brooklyn/rest/api/AdjunctApi.java

+            @ApiResponse(code = 404, message = "Could not find application or entity"),
+            @ApiResponse(code = 400, message = "Type is not a suitable adjunct")
+    })
+    public AdjunctSummary addAdjunct(


I compared the EntityApi.addChildren to this. It would be really nice if one could just pass in YAML for the adjunct being added (similar to addChildren).

How feasible would that be? I'm guessing our yaml parser just doesn't have the right entry points for that.

Perhaps we should mark this as @Beta (including in the swagger description text, and come back to it in a v2?

have added comment about supporting yaml. this isn't an addition, it's just a migration, so i wouldn't deprecate until we have the yaml parser for it. (with yoml it should be easy to plug in to the right place. not too hard even so.)

aledsage · 2017-10-02T12:22:24Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtils.java

+     * 
+     * @throws 404 or 412 (unless input is null in which case output is null) */
+    public EntityAdjunct getAdjunct(Entity entity, String adjunct) {
+        if (adjunct==null) return null;


Feels wrong to return null if the adjunct param is null.
Looking at the uses of it, that results in a subsequent NPE (i.e. a 500 response code).
I'd change this method to return an appropriate 4xx response code (not sure which one - writing this offline!)

I know that in this PR you've just followed the same convention as for getPolicy(), getEntity(), etc. But I think they are wrong as well! Happy for us to do that in a different PR.

let's make it separate just in case -- agree no use case for null input. i'd probably make it a fail-fast NPE (500 or 412 precondition failed) rather than a 404.

aledsage · 2017-10-02T12:22:46Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+        EntityAdjunct adjunct = brooklyn().getAdjunct(application, entityToken, adjunctId);
+        if (adjunct instanceof Policy) ((Policy)adjunct).resume();
+        else if (adjunct instanceof Feed) ((Feed)adjunct).resume();
+        else throw WebResourceUtils.badRequest("%s does not support start/resume", adjunct);


Do you think enrichers should support suspend/resume? If so, worth adding a TODO here.

aledsage · 2017-10-02T12:23:15Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+        return result;
+    }
+
+    // TODO support parameters  ?show=value,summary&name=xxx &format={string,json,xml}


I'd move that comment to the AdjunctsApi interface, rather than in the impl (given it's about the api rather than the implementation).

already there

aledsage · 2017-10-02T12:25:26Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+    public String getConfig(String application, String entityToken, String adjunctToken, String configKeyName) {
+        EntityAdjunct adjunct = brooklyn().getAdjunct(application, entityToken, adjunctToken);
+        Set<ConfigKey<?>> cki = adjunct.config().findKeysDeclared(ConfigPredicates.nameSatisfies(Predicates.equalTo(configKeyName)));
+        if (cki.isEmpty()) throw WebResourceUtils.notFound("Cannot find config key '%s' in policy '%s' of entity '%s'", configKeyName, adjunctToken, entityToken);


Should be "in adjunct ", rather than "in policy ".

aledsage · 2017-10-02T12:25:35Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/AdjunctResource.java

+
+        EntityAdjunct adjunct = brooklyn().getAdjunct(entity, adjunctToken);
+        Set<ConfigKey<?>> cki = adjunct.config().findKeysDeclared(ConfigPredicates.nameSatisfies(Predicates.equalTo(configKeyName)));
+        if (cki.isEmpty()) throw WebResourceUtils.notFound("Cannot find config key '%s' in policy '%s' of entity '%s'", configKeyName, adjunctToken, entityToken);


Should be "in adjunct ", rather than "in policy ".

aledsage · 2017-10-02T12:26:01Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/ConfigTransformer.java

+    public ConfigSummary transform() {
+        MutableMap.Builder<String, URI> lb = new MutableMap.Builder<String, URI>();
+
+        if (ub!=null && entity!=null) {


Feels like a programming error if someone called includeLinks (to supply a UriBuilder), but the entity is null. Perhaps fail-fast rather than just missing out the links? Otherwise our unit tests may well all pass if such a programming error is introduced.

there might be a context where someone wants to include whatever links are possible but an entity may or may not be present. i wouldn't say it's necessarily a programming error. if inclusion in links is wanted unit tests should check it explicitly rather than constrain this method to fail-fast.

i'll update javadoc on includeLinks methods

aledsage · 2017-10-02T12:26:32Z

rest/rest-api/src/main/java/org/apache/brooklyn/rest/api/PolicyApi.java

@@ -36,6 +36,8 @@
 @Api("Entity Policies")
 @Produces(MediaType.APPLICATION_JSON)
 @Consumes(MediaType.APPLICATION_JSON)
+/** @deprecated since 0.12.0 use AdjunctApi */


Shouldn't this be "since 0.13.0"?

(Please search https://github.com/apache/brooklyn-server/pull/821/files for that text)

aledsage · 2017-10-02T12:27:40Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtils.java

+        }
+        for (Feed p: ((EntityInternal)entity).feeds()) {
+            if (adjunct.equals(p.getDisplayName())) return p;
+        }


Depends whether we worry about a display name being the same as an id anywhere - and thus what guarantees we're trying to give.

I probably agree @tbouron. If we do want to keep them separate, we should iterate over all ids (policies, enrichers, feeds) before going on to display names.

…r in swagger

…lasses deprecating more methods which need to change or should go will need to add FeedSpec in future to bring it fully in line

…date other deprecated

ahgittin · 2017-10-03T12:11:27Z

one question from above to call out special @tbouron (given how github hides things) regarding the list method which takes argument

@QueryParam("adjunctType") final String adjunctType

you've suggested type but that normally refers to registered type which is wrong here; i think either leave as is, or change to supertype or adjunctKind. wdyt?

ahgittin · 2017-10-03T12:19:26Z

left it as adjunctType for now - could evolve to supertype in future

tests added

that's everything addresses i think

tbouron · 2017-10-03T12:20:50Z

@ahgittin Sorry, I did miss your comment. I was suggesting type to be consistent with the DELETE method. I guess adjunctType is fine for now (superType is really confusing for me) but I would really like to fix this in the future, across all endpoints

ahgittin · 2017-10-05T09:08:20Z

@tbouron totally forgot about your point karaf is missing this; have fixed that in #854 if you could take a quick look

tbouron · 2017-10-05T09:24:17Z

@ahgittin I'll review #854 now. Although, there is another point that is missing from this PR: Swagger still don't show the deprecation warning for the entity policy endpoints.

For example, here is the one for /v1/locations:

ahgittin added 2 commits September 13, 2017 16:52

Merge branch 'bundle-rest-api' into adjunct-rest-api-and-highlights-2

80a4c25

Merge branch 'highlights-adjuncts' into adjunct-rest-api-and-highligh…

2860af5

…ts-2

add adjunct REST API, deprecating policy endpoint

45261c7

ahgittin force-pushed the adjunct-rest-api-and-highlights branch from 07247e8 to 45261c7 Compare September 13, 2017 16:35

ahgittin added 2 commits September 25, 2017 11:29

Merge branch 'master' into adjunct-rest-2

62bcbc5

Big changes to config key construction, following on from previous deprecation

clean up config rest utils following deprecation of config classes

83c3d50

ahgittin force-pushed the adjunct-rest-api-and-highlights branch from 4a148f9 to 83c3d50 Compare September 25, 2017 12:08

ahgittin mentioned this pull request Sep 25, 2017

REST call to get activities on an adjunct #836

Merged

tbouron requested changes Sep 28, 2017

View reviewed changes

aledsage reviewed Oct 2, 2017

View reviewed changes

ahgittin added 4 commits October 3, 2017 09:17

Merge branch 'master' into adjunct-rest-api-and-highlights

4f4507a

add deprecation notes to description for REST endpoints so they appea…

e016053

…r in swagger

steps toward bringing FeedSupport in line with other AdjunctSupport c…

dd4f05e

…lasses deprecating more methods which need to change or should go will need to add FeedSpec in future to bring it fully in line

address PR comments, note deprecation now since 0.13.0, and remove/up…

28dbb83

…date other deprecated

ahgittin force-pushed the adjunct-rest-api-and-highlights branch from e2c3f4c to 28dbb83 Compare October 3, 2017 11:29

add adjuncts rest test, and minor fixes

d5d72cb

ahgittin added a commit to ahgittin/brooklyn-server that referenced this pull request Oct 3, 2017

This closes apache#821

c177a7c

asfgit merged commit d5d72cb into apache:master Oct 3, 2017

ahgittin mentioned this pull request Oct 5, 2017

add adjunct endpoint to karaf config #854

Merged

REST API for accessing adjuncts (including highlights) #821

REST API for accessing adjuncts (including highlights) #821

Conversation

ahgittin commented Sep 13, 2017 • edited

ahgittin commented Sep 13, 2017

ahgittin commented Sep 25, 2017

tbouron commented Sep 28, 2017

ahgittin commented Sep 28, 2017

tbouron commented Sep 28, 2017

tbouron left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbouron Sep 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbouron commented Sep 28, 2017

aledsage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahgittin commented Oct 3, 2017

ahgittin commented Oct 3, 2017

tbouron commented Oct 3, 2017

ahgittin commented Oct 5, 2017

tbouron commented Oct 5, 2017

ahgittin commented Sep 13, 2017 •

edited

tbouron left a comment •

edited

tbouron Sep 28, 2017 •

edited