Skip to content
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.

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

Add support for managing enrichers within the catalog #638

Merged
merged 1 commit into from
May 9, 2017

Conversation

tbouron
Copy link
Member

@tbouron tbouron commented Apr 14, 2017

This allows enrichers to be registered within the Brooklyn catalog, same as entities or policies. It allows enrichers' registration via:

  • @Catalog annotation on the java class
  • YAML with an itemType: enricher

This also adds 3 endpoints to the /v1/catalog REST API:

  • GET /v1/catalog/enrichers returns the list of all registered enrichers
  • GET /v1/catalog/enrichers/<enricherId>/<version> returns the detail of a specific enricher
  • DELETE /v1/catalog/enrichers/<enricherId>/<version> details a specific enricher

PS: If the PR is too big, I can split up to multiples PRs. Hopefully the commit's messages are pretty clear

@aledsage
Copy link
Contributor

@tbouron see brooklyn-server/policy/src/main/resources/catalog.bom - I think we'd need to list the enrichers in a .bom file, otherwise we won't list them when running in karaf (because we won't classpath-scan for annotated enrichers across all bundles, I believe).

@tbouron
Copy link
Member Author

tbouron commented Apr 18, 2017

@aledsage Tried to add enricher definitions in the /src/main/resources/catalog.bom of each bundle where the enricher is located, plus in karaf/init/src/main/resources/catalog-classes.bom but so far, they are not registered when I start Brooklyn in Karaf mode.

Am I doing something wrong?

@tbouron tbouron force-pushed the feature/catalog-enrichers branch 2 times, most recently from e07c0e1 to ba561e9 Compare May 5, 2017 10:03
Copy link
Contributor

@geomacy geomacy left a comment

Choose a reason for hiding this comment

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

Looks good to me. A couple of micro observations.

@@ -116,6 +120,8 @@ public static CatalogItemSummary catalogItemSummary(BrooklynRestResourceUtils b,
return catalogEntitySummary(b, item, ub);
case POLICY:
return catalogPolicySummary(b, item, ub);
case ENRICHER:
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

@@ -55,7 +56,7 @@
* (or until another process manually sets {@link Attributes#SERVICE_STATE_ACTUAL} to {@value Lifecycle#ON_FIRE},
* which this enricher will not clear until all problems have gone away)
*/
//@Catalog(name="Service Failure Detector", description="HA policy for deteting failure of a service")
@Catalog(name="Service Failure Detector", description="Emits a new sensor if the current entity has an issue")
Copy link
Contributor

Choose a reason for hiding this comment

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

"has an issue" seems too general, would simply if the current entity fails be better?


//Would ideally re-use the PolicySpecResolver
//but it is CAMP specific and there is no easy way to get hold of it.
Object policies = checkNotNull(plan.getCustomAttributes().get(BasicBrooklynCatalog.ENRICHERS_KEY), "enricher config");
Copy link
Contributor

Choose a reason for hiding this comment

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

variable name is still policies

static EnricherSpec<?> createEnricherSpec(String yamlPlan, BrooklynClassLoadingContext loader, Set<String> encounteredCatalogTypes) {
DeploymentPlan plan = makePlanFromYaml(loader.getManagementContext(), yamlPlan);

//Would ideally re-use the PolicySpecResolver
Copy link
Contributor

Choose a reason for hiding this comment

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

PolicySpecResolver in the comment isn't right

@tbouron tbouron force-pushed the feature/catalog-enrichers branch from ba561e9 to 3320a51 Compare May 9, 2017 16:01
@tbouron
Copy link
Member Author

tbouron commented May 9, 2017

Thanks for the review @geomacy! I addressed your comments and rebased on the latest master

@geomacy
Copy link
Contributor

geomacy commented May 9, 2017

Thanks @tbouron, that's great, merging now.

@geomacy
Copy link
Contributor

geomacy commented May 9, 2017

Actually can you squash the commits?

…scanned and YAML parsed, same as entities or policies)
@tbouron tbouron force-pushed the feature/catalog-enrichers branch from 3320a51 to 02305e7 Compare May 9, 2017 16:21
@tbouron
Copy link
Member Author

tbouron commented May 9, 2017

@geomacy done!

@geomacy
Copy link
Contributor

geomacy commented May 9, 2017

Thanks, merging now

@asfgit asfgit merged commit 02305e7 into apache:master May 9, 2017
asfgit pushed a commit that referenced this pull request May 9, 2017
Add support for managing enrichers within the catalog

This allows enrichers to be registered within the Brooklyn catalog, same as entities or policies. It allows enrichers' registration via:
- `@Catalog` annotation on the java class
- YAML with an `itemType: enricher`

This also adds 3 endpoints to the `/v1/catalog` REST API:
- `GET /v1/catalog/enrichers` returns the list of all registered enrichers
- `GET /v1/catalog/enrichers/<enricherId>/<version>` returns the detail of a specific enricher
- `DELETE /v1/catalog/enrichers/<enricherId>/<version>` details a specific enricher

_PS: If the PR is too big, I can split up to multiples PRs. Hopefully the commit's messages are pretty clear_
@tbouron tbouron deleted the feature/catalog-enrichers branch May 9, 2017 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants