Skip to content

Added highlights to policy endpoint#813

Merged
asfgit merged 1 commit intoapache:masterfrom
Graeme-Miller:add_highlight_policy
Sep 13, 2017
Merged

Added highlights to policy endpoint#813
asfgit merged 1 commit intoapache:masterfrom
Graeme-Miller:add_highlight_policy

Conversation

@Graeme-Miller
Copy link
Copy Markdown
Contributor

Policies now return highlights from the REST API. Highlights are a map of ids -> (description, time, task id). They are used to expose more information about a policy, for example the last action or the triggers for a policy.

I have also updated the rebind code such that policies can have there highlights re-bound.

In the future highlights will be extended to all adjuncts.

@ahgittin
Copy link
Copy Markdown
Contributor

looks good, nice and straightforward. will run some tests then merge assuming all good.

Copy link
Copy Markdown
Member

@m4rkmckenna m4rkmckenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minors

General
Im struggling to see how a client will use this information. Is it just an arbitrary selection of stuff that the adjunct wishes to surface that will change from adjunct to adjunct? If so i think the highlight should probably contain more information to describe what it is

* under the License.
*/
package org.apache.brooklyn.api.objs;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing java doc

package org.apache.brooklyn.api.objs;

public class HighlightTuple {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would AdjunctHighlight be a better name?

if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

HighlightTuple that = (HighlightTuple) o;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id personally use guava Objects.equalsas its already a dependency


@Override
public int hashCode() {
int result = description != null ? description.hashCode() : 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above Objects.hashCode

@ahgittin
Copy link
Copy Markdown
Contributor

works a treat, did a sample impl on ServiceRestarter, and tested persistence too. merging this. i'll open a follow-on PR which will address the points here and show how it's used in that class.

@asfgit asfgit merged commit 962b777 into apache:master Sep 13, 2017
asfgit pushed a commit that referenced this pull request Sep 13, 2017
@ahgittin
Copy link
Copy Markdown
Contributor

@m4rkmckenna re your general comment, a human client will use this to understand what's going on. currently adjuncts (policies etc) are opaque unless you look at logs or tasks (or code). highlights are an arbitrary set of summary info -- although with some conventions -- which let an adjunct communicate to a user at a high level what it has been doing. on the premise that a little bit of sensibly distilled info is better than lots of raw info -- but sensible distillation does depend on the adjunct author a bit. (i'd like to have activities properly tagged to adjuncts as well, so we can also expose lots of "raw" info.)

your point about javadoc is a good one so people know how to use these -- added in #818.

i prefer the name HighlightTuple as there is nothing adjunct-specific about them, just they are only used for adjuncts.

i also prefer Objects.equal and hashCode but if your IDE generates this for you or it has already been written i don't think it's worth the time to change ... doesn't create any value for user!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants