You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by GitBox <gi...@apache.org> on 2022/02/07 11:12:25 UTC
[GitHub] [brooklyn-server] algairim opened a new pull request #1298: Call effector if value of sensor being watched changes only
algairim opened a new pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298
Signed-off-by: Mykola Mandra <my...@cloudsoft.io>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim closed pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim closed pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] duncangrant commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801852802
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +80,29 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
Review comment:
Good point - I missed this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#issuecomment-1033614739
No need in this change, I should have used `enricher.suppressDuplicates` option available in all enrichers instead, as @duncangrant suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] duncangrant edited a comment on pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
duncangrant edited a comment on pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#issuecomment-1033536440
> > @algairim Why are you making this change? I think you can already get this behaviour by using an enricher with suppress duplicates. Is this just to make it easier to do this?
> > Should we use a suppress duplicates flag so that anyone relying on the current behaviour doesn't see their blueprint stop working.
>
> @duncangrant , which enricher are you referring to and how can I combine it with this policy?
Propagator with enricher.suppressDuplicates true would work but I don't think that would work with the same target and source entity so maybe there isn't an option that works well here. Also most sensors have a suppressDuplicates flag so you might want to check that for your use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#issuecomment-1032633844
> @algairim Why are you making this change? I think you can already get this behaviour by using an enricher with suppress duplicates. Is this just to make it easier to do this?
>
> Should we use a suppress duplicates flag so that anyone relying on the current behaviour doesn't see their blueprint stop working.
@duncangrant , which enricher are you referring to and how can I combine it with this policy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] ahgittin commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r802482442
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +80,29 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
subscriptions().subscribe(producer, sensor, this);
LOG.debug("{} subscribed to {} events on {}", new Object[]{this, sensor, entity});
}
@Override
public void onEvent(SensorEvent<Object> event) {
- final Effector<?> eff = getEffectorNamed(getConfig(EFFECTOR)).get();
+ final Object newValue = event.getValue();
+
+ synchronized (lock) {
+ LOG.debug("lastValue='{}', newValue='{}'", lastValue, newValue);
+ if (newValue instanceof Comparable && newValue.equals(lastValue)) {
Review comment:
can we add a `ConfigKey SUPPRESS_DUPLICATES` (this is used elsewhere), and only exit here if that is true; it can default to true but at least that way if there is a legacy requirement somewhere to act even if values don't change then we can support this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] ahgittin commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r802478109
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -67,7 +67,7 @@
.build();
private AttributeSensor<Object> sensor;
-
+ private Object lastValue;
Review comment:
i think this will be included in rebind anyway (even without being set in `setEntity`). you could mark it `transient` to exclude it from rebind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801657078
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -67,7 +67,7 @@
.build();
private AttributeSensor<Object> sensor;
-
+ private Object lastValue;
Review comment:
Agree, should be synchronised. I completely forgot about rebind!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801670660
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -67,7 +67,7 @@
.build();
private AttributeSensor<Object> sensor;
-
+ private Object lastValue;
Review comment:
Line 82 addresses rebind actually. That is where we know the initial value to ocmpare to new from events.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] duncangrant commented on pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
duncangrant commented on pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#issuecomment-1032618705
@algairim Why are you making this change? I think you can already get this behaviour by using an enricher with suppress duplicates. Is this just to make it easier to do this?
Should we use a suppress duplicates flag so that anyone relying on the current behaviour doesn't see their blueprint stop working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] duncangrant commented on pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
duncangrant commented on pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#issuecomment-1033536440
> > @algairim Why are you making this change? I think you can already get this behaviour by using an enricher with suppress duplicates. Is this just to make it easier to do this?
> > Should we use a suppress duplicates flag so that anyone relying on the current behaviour doesn't see their blueprint stop working.
>
> @duncangrant , which enricher are you referring to and how can I combine it with this policy?
Propagator with enricher.suppressDuplicates true would work but I don't think that would work with the same target and source entity so maybe there isn't an option that works well here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801691125
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +80,29 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
Review comment:
This is where last known value is expected to be recovered after rebind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] ahgittin commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r802480710
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +80,29 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
subscriptions().subscribe(producer, sensor, this);
LOG.debug("{} subscribed to {} events on {}", new Object[]{this, sensor, entity});
}
@Override
public void onEvent(SensorEvent<Object> event) {
- final Effector<?> eff = getEffectorNamed(getConfig(EFFECTOR)).get();
+ final Object newValue = event.getValue();
+
+ synchronized (lock) {
Review comment:
this synch block seems pointless to me, if there were duplicate events coming in concurrently there is no guarantee which one is set first.
however i believe subscription delivery is already synched to ensure in-order delivery anyway.
so remove this synch block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801678719
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +79,26 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
subscriptions().subscribe(producer, sensor, this);
LOG.debug("{} subscribed to {} events on {}", new Object[]{this, sensor, entity});
}
@Override
public void onEvent(SensorEvent<Object> event) {
- final Effector<?> eff = getEffectorNamed(getConfig(EFFECTOR)).get();
+ final Object newValue = event.getValue();
+ LOG.debug("lastValue='{}', newValue='{}'", lastValue, newValue);
+ if (String.valueOf(newValue).equals(lastValue)) {
Review comment:
9cfe27fdca482df87febc49aa6a8b071512a44fa
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +79,26 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
subscriptions().subscribe(producer, sensor, this);
LOG.debug("{} subscribed to {} events on {}", new Object[]{this, sensor, entity});
}
@Override
public void onEvent(SensorEvent<Object> event) {
- final Effector<?> eff = getEffectorNamed(getConfig(EFFECTOR)).get();
+ final Object newValue = event.getValue();
+ LOG.debug("lastValue='{}', newValue='{}'", lastValue, newValue);
+ if (String.valueOf(newValue).equals(lastValue)) {
Review comment:
- [x] You are right, I'll cast this to `Comparable`, left this from experimentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#issuecomment-1033574969
> From our discussion I think this is useful but I think it needs a suppress duplicates flag in line with enrichers and sensor feeds rather than always do this.
I did not notice that we have this option in all our enrichers, let me check my use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] duncangrant commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801643384
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +79,26 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
subscriptions().subscribe(producer, sensor, this);
LOG.debug("{} subscribed to {} events on {}", new Object[]{this, sensor, entity});
}
@Override
public void onEvent(SensorEvent<Object> event) {
- final Effector<?> eff = getEffectorNamed(getConfig(EFFECTOR)).get();
+ final Object newValue = event.getValue();
+ LOG.debug("lastValue='{}', newValue='{}'", lastValue, newValue);
+ if (String.valueOf(newValue).equals(lastValue)) {
Review comment:
I don't understand why you're converting to a string here - sensors can hold any object type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801670660
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -67,7 +67,7 @@
.build();
private AttributeSensor<Object> sensor;
-
+ private Object lastValue;
Review comment:
Line 82 addresses rebind actually. That is where we know the initial value to compare to new ones from events.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801692459
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -67,7 +67,7 @@
.build();
private AttributeSensor<Object> sensor;
-
+ private Object lastValue;
Review comment:
08aa7c3447658c5c6e5f518bca631b33526fa9d5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] algairim commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
algairim commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801647597
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -79,18 +79,26 @@ public void setEntity(EntityLocal entity) {
LOG.debug("Defaulting to producer==self for {}, on entity {}", this, entity);
producer = entity;
}
+ lastValue = entity.sensors().get(sensor);
subscriptions().subscribe(producer, sensor, this);
LOG.debug("{} subscribed to {} events on {}", new Object[]{this, sensor, entity});
}
@Override
public void onEvent(SensorEvent<Object> event) {
- final Effector<?> eff = getEffectorNamed(getConfig(EFFECTOR)).get();
+ final Object newValue = event.getValue();
+ LOG.debug("lastValue='{}', newValue='{}'", lastValue, newValue);
+ if (String.valueOf(newValue).equals(lastValue)) {
Review comment:
- [ ] You are right, I'll cast this to `Comparable`, left this from experimentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] duncangrant commented on a change in pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
duncangrant commented on a change in pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#discussion_r801638970
##########
File path: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSensorChange.java
##########
@@ -67,7 +67,7 @@
.build();
private AttributeSensor<Object> sensor;
-
+ private Object lastValue;
Review comment:
This won't survive rebind and I'm not sure it's thread safe (it might be - I'd need to dig into the parent's code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-server] ahgittin commented on pull request #1298: Call effector if value of sensor being watched changes only
Posted by GitBox <gi...@apache.org>.
ahgittin commented on pull request #1298:
URL: https://github.com/apache/brooklyn-server/pull/1298#issuecomment-1033579241
a few changes requested, otherwise looks good:
* `transient lastValue`
* no `synchronized`
* add `SUPPRESS_DUPLICATES` config key
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org