Add series of meta effectors for composite operations by grkvlt · Pull Request #605 · apache/brooklyn-server

grkvlt · 2017-03-22T16:58:56Z

Adds ComposeEffector, SequenceEffector, TransformEffector and LoopEffector for orchestrating effector calls in blueprints.

Also ScheduledEffectorPolicy and PeriodicEffectorPolicy policies to execute an effector at a scheduled time or periodically.

TODO Add tests for each new effector type and basic documentation

robertgmoss · 2017-03-23T11:18:47Z

core/src/main/java/org/apache/brooklyn/core/effector/composite/AbstractCompositeEffector.java

+    @Override
+    public void apply(EntityLocal entity) {
+        Maybe<Effector<?>> effectorMaybe = entity.getEntityType().getEffectorByName(effector.getName());
+        if (!effectorMaybe.isAbsentOrNull()) {


All of the logic in this has been commented out! If none is required, makes overriding the entire method superfluous.

robertgmoss · 2017-03-23T11:26:07Z

core/src/main/java/org/apache/brooklyn/core/effector/composite/AbstractCompositeEffector.java

+                    throw new IllegalArgumentException("Effector parameter cannot be parsed: " + effectorDetails);
+                }
+                effectorName = Iterables.getOnlyElement(keys);
+            } else {


what about instanceof List?

This is the individual effector elements of the list of effectors, they can only be a String or a Map

I mean what if someone passes a list - effectorName = Strings.toString(effectorDetails); does not feel like the right way to handle that

But why would they do that? They would be creating a list of lists, as in the third option below, and if someone does that, then I'm happy for things to just break...

compose: - name1 # string - name2: # map key: value - [ whyWouldYouDoThis ] # list

Indeed, it would clearly be a mistake - prefer to fail fast, so perhaps throwing an exception rather than allowing it to pass by calling toString()

geomacy · 2017-03-23T16:41:51Z

@grkvlt there should be some unit tests at least along with these, can you add some?

robertgmoss · 2017-03-24T10:18:09Z

core/src/main/java/org/apache/brooklyn/core/effector/composite/ComposeEffector.java

+
+        public Body(Effector<?> eff, ConfigBag params) {
+            super(eff, params);
+            Preconditions.checkNotNull(params.getAllConfigRaw().get(COMPOSE.getName()), "Effector names must be supplied when defining this effector");


Should this be Preconditions.checkNotNull(params.get(COMPOSE), "compose must be supplied when defining a compose effector: %s", params)?

Yes, I'll fix all of these to have better error messages.

robertgmoss · 2017-03-24T10:18:13Z

core/src/main/java/org/apache/brooklyn/core/effector/composite/LoopEffector.java

+
+        public Body(Effector<?> eff, ConfigBag params) {
+            super(eff, params);
+            Preconditions.checkNotNull(params.getAllConfigRaw().get(LOOP.getName()), "Effector names must be supplied when defining this effector");


Should this be Preconditions.checkNotNull(params.get(LOOP), "loop must be supplied when defining a loop effector: %s", params)?

robertgmoss · 2017-03-24T10:18:18Z

core/src/main/java/org/apache/brooklyn/core/effector/composite/SequenceEffector.java

+
+        public Body(Effector<?> eff, ConfigBag params) {
+            super(eff, params);
+            Preconditions.checkNotNull(params.getAllConfigRaw().get(SEQUENCE.getName()), "Effector names must be supplied when defining this effector");


Should this be Preconditions.checkNotNull(params.get(SEQUENCE), "sequence must be supplied when defining a sequence effector: %s", params)?

robertgmoss · 2017-03-24T10:18:23Z

core/src/main/java/org/apache/brooklyn/core/effector/composite/TransformEffector.java

+
+        public Body(Effector<?> eff, ConfigBag params) {
+            super(eff, params);
+            Preconditions.checkNotNull(params.getAllConfigRaw().get(FUNCTION.getName()), "Function must be supplied when defining this effector");


Should this be Preconditions.checkNotNull(params.get(FUNCTION), "function must be supplied when defining a function effector: %s", params)?

Also what about INPUT?

robertgmoss · 2017-03-27T13:05:09Z

policy/src/main/java/org/apache/brooklyn/policy/action/PeriodicEffectorPolicy.java

+
+    protected long delay;
+
+    private final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor();


Any reason not to inherit the ScheduledExecutorService of the parent class and use that?

Graeme-Miller · 2017-05-05T12:32:44Z

@grkvlt this PR is rather large, and missing tests. I suggest it is closed for now and we work out a way to break this down.

Some ideas for how to break it down:
*) Separate PR's per effector would be nice
*) New sensor types as separate PRs (e.g. AbstractCompositeEffector/PeriodicEffectorPolicy.java)
*) Refactors as separate PRs (Moving AddSensor)

This should make it easier to review and merge.

What are your thoughts on this?

grkvlt · 2017-05-05T15:20:00Z

@Graeme-Miller I think once the tests are added it should be OK, as the individual effectors are all reliant on a single parent abstract class, so they fit together. I will work with @robertgmoss on getting this done and reviewed.

…in policy

nakomis · 2019-11-26T11:15:01Z

@grkvlt Unfortunately it looks like this branch has gone stale since you updated it. Can you have a look to see if it is still relevant (I suspect it is), and rebase against master

Thanks

robertgmoss reviewed Mar 23, 2017

View reviewed changes

robertgmoss reviewed Mar 24, 2017

View reviewed changes

grkvlt force-pushed the feature/composite-effectors branch 2 times, most recently from b60b332 to e3a936f Compare March 27, 2017 10:47

robertgmoss reviewed Mar 27, 2017

View reviewed changes

This was referenced Mar 28, 2017

[WIP] Add support for JSR-223 scripting of effectors #153

Closed

Documentation for new ScriptEffector apache/brooklyn-docs#72

Closed

grkvlt force-pushed the feature/composite-effectors branch 8 times, most recently from ca62777 to de72251 Compare April 6, 2017 02:40

grkvlt force-pushed the feature/composite-effectors branch 4 times, most recently from caac7c4 to 9413b4b Compare April 11, 2017 15:16

grkvlt force-pushed the feature/composite-effectors branch from d6a09d1 to 8f88cfb Compare May 15, 2017 12:41

grkvlt force-pushed the feature/composite-effectors branch 2 times, most recently from 6ac8152 to 17f7e0f Compare June 5, 2017 15:31

Add series of meta effectors for composite operations

7b53143

grkvlt and others added 28 commits June 29, 2017 15:47

Minor tidy up of composite effectors

c228e49

Better check of boolean values in policies

131d24f

Fix bug where wrong original effrector is called by replace

35463eb

Use JsonIdentityInfo annotation on all HasId domain objects

14a714d

Add debug logging and an atomic state check to scheduler policies

1fba768

Add start time config for periodic policy

3033b4a

Add policy event handler mutex and fix sensor check

0c735ae

Display delay as time string in periodic policy

ec1add2

Log return values from composite effectors

e373b59

Update JSON serialisation annotations

fc37f15

Make policy config resolve properly

5c7cba3

Simplify collection functions

78d90d2

Update ServiceStateLogic to handle STARTING differently

572075e

Add mutex to meta effectors and fix resolution of effector arguments …

99f006f

…in policy

Adds wait config to scheduled and periodic policies

f8c4004

Updating task submission in policy executor context

8d1ad1e

Better handling of wait time and synchronisation in scheduler policies

9aeeee2

Use Date object in scheduling policies for time config

41dc2cc

Revert to string time representation and DateFormat

cf45d47

Added new SetSensorEffector to set attributes on entities

4dbbe72

Accept now or immediately as time specification for policy scheduling

2c12d9e

Update Javadoc and remove @beta annotations from effectors

12c5970

Add parallel effector and parallelize loop effector

8ab2a8b

Use deep resolving task for policy effector args

453d103

Add new RepeatEffector implementation

275b547

Use config key builder for composite effectors

6c3bcea

Add removal strategy for dynamic cluster using predicates

55087fa

New effector to remove entities

7f28d19

grkvlt force-pushed the feature/composite-effectors branch from 17f7e0f to 7f28d19 Compare June 29, 2017 14:48


		protected long delay;

		private final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor();

Conversation

grkvlt commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geomacy commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Graeme-Miller commented May 5, 2017

Uh oh!

grkvlt commented May 5, 2017

Uh oh!

nakomis commented Nov 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

grkvlt commented Mar 22, 2017 •

edited

Loading