brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From drigod...@apache.org
Subject [3/4] brooklyn-server git commit: Fixes the deduplication logic in AbstractEnricher
Date Wed, 03 May 2017 08:57:25 GMT
Fixes the deduplication logic in AbstractEnricher

Could incorrectly deduplicate (for example result in setting the same value twice) when the
value is set from parallel threads.


Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/28dc13e8
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/28dc13e8
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/28dc13e8

Branch: refs/heads/master
Commit: 28dc13e888edb77c9010c8ab13a56d7782f50e28
Parents: 4974948
Author: Svetoslav Neykov <svetoslav.neykov@cloudsoftcorp.com>
Authored: Fri Apr 7 19:31:04 2017 +0300
Committer: Svetoslav Neykov <svetoslav.neykov@cloudsoftcorp.com>
Committed: Mon Apr 24 17:31:46 2017 +0300

----------------------------------------------------------------------
 .../core/enricher/AbstractEnricher.java         | 36 ++++++++++++++++----
 .../ApplicationLifecycleStateStressTest.java    |  1 -
 .../entity/ApplicationLifecycleStateTest.java   |  9 +++--
 3 files changed, 33 insertions(+), 13 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/28dc13e8/core/src/main/java/org/apache/brooklyn/core/enricher/AbstractEnricher.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/enricher/AbstractEnricher.java b/core/src/main/java/org/apache/brooklyn/core/enricher/AbstractEnricher.java
index f957b18..39b23ba 100644
--- a/core/src/main/java/org/apache/brooklyn/core/enricher/AbstractEnricher.java
+++ b/core/src/main/java/org/apache/brooklyn/core/enricher/AbstractEnricher.java
@@ -22,7 +22,6 @@ import static com.google.common.base.Preconditions.checkState;
 
 import java.util.Map;
 
-import org.apache.brooklyn.api.entity.EntityLocal;
 import org.apache.brooklyn.api.mgmt.rebind.RebindSupport;
 import org.apache.brooklyn.api.mgmt.rebind.mementos.EnricherMemento;
 import org.apache.brooklyn.api.sensor.AttributeSensor;
@@ -36,7 +35,9 @@ import org.apache.brooklyn.core.entity.EntityInternal;
 import org.apache.brooklyn.core.mgmt.rebind.BasicEnricherRebindSupport;
 import org.apache.brooklyn.core.objs.AbstractEntityAdjunct;
 import org.apache.brooklyn.util.core.flags.TypeCoercions;
+import org.apache.brooklyn.util.guava.Maybe;
 
+import com.google.common.base.Function;
 import com.google.common.base.Objects;
 import com.google.common.collect.Maps;
 
@@ -49,6 +50,27 @@ public abstract class AbstractEnricher extends AbstractEntityAdjunct implements
             "enricher.suppressDuplicates",
             "Whether duplicate values published by this enricher should be suppressed");
 
+    private static class DeduplicatingAttributeModifier<T> implements Function<T,
Maybe<T>> {
+        public static <T> Function<T, Maybe<T>> create(T newVal) {
+            return new DeduplicatingAttributeModifier<T>(newVal);
+        }
+
+        private DeduplicatingAttributeModifier(T newVal) {
+            this.newValue = newVal;
+        }
+
+        private T newValue;
+
+        @Override
+        public Maybe<T> apply(T oldValue) {
+            if (Objects.equal(oldValue, newValue)) {
+                return Maybe.absent("Skipping update, values equal");
+            } else {
+                return Maybe.of(newValue);
+            }
+        }
+    }
+
     private final EnricherDynamicType enricherType;
     protected Boolean suppressDuplicates;
 
@@ -83,7 +105,7 @@ public abstract class AbstractEnricher extends AbstractEntityAdjunct implements
     }
 
     @Override
-    public void setEntity(EntityLocal entity) {
+    public void setEntity(@SuppressWarnings("deprecation") org.apache.brooklyn.api.entity.EntityLocal
entity) {
         super.setEntity(entity);
         Boolean suppressDuplicates = getConfig(SUPPRESS_DUPLICATES);
         if (suppressDuplicates!=null) 
@@ -102,18 +124,18 @@ public abstract class AbstractEnricher extends AbstractEntityAdjunct
implements
             return;
         }
         if (val == Entities.REMOVE) {
-            ((EntityInternal)entity).removeAttribute((AttributeSensor<T>) sensor);
+            ((EntityInternal)entity).sensors().remove((AttributeSensor<T>) sensor);
             return;
         }
         
         T newVal = TypeCoercions.coerce(val, sensor.getTypeToken());
         if (sensor instanceof AttributeSensor) {
+            AttributeSensor<T> attribute = (AttributeSensor<T>)sensor;
             if (Boolean.TRUE.equals(suppressDuplicates)) {
-                T oldValue = entity.getAttribute((AttributeSensor<T>)sensor);
-                if (Objects.equal(oldValue, newVal))
-                    return;
+                entity.sensors().modify(attribute, DeduplicatingAttributeModifier.create(newVal));
+            } else {
+                entity.sensors().set(attribute, newVal);
             }
-            entity.sensors().set((AttributeSensor<T>)sensor, newVal);
         } else { 
             entity.sensors().emit(sensor, newVal);
         }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/28dc13e8/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java
b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java
index a6a6f9f..02f8ec2 100644
--- a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java
+++ b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java
@@ -100,7 +100,6 @@ public class ApplicationLifecycleStateStressTest extends ApplicationLifecycleSta
         super.testWrongSensorInitValue();
     }
 
-    @Test(groups="Broken")
     @Override
     public void testAbstractEnricherDeduplicationBroken() {
         super.testAbstractEnricherDeduplicationBroken();

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/28dc13e8/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java
b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java
index 7afe2f6..e8a3eee 100644
--- a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java
+++ b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java
@@ -365,12 +365,11 @@ public class ApplicationLifecycleStateTest extends BrooklynMgmtUnitTestSupport
{
     }
 
     /**
-     * The deduplication logic in AbstractEnricher does not work for parallel invocations.
-
-     * Indeterministic, fails a couple of times per 100 invocations when run with "mvn test"
in the
-     * brooklyn-itest docker container.
+     * The deduplication logic in AbstractEnricher previously did not work for parallel invocations.
+     * It used to do a get and then a compare, so another thread could change the value between
+     * those two operations.
      */
-    @Test(groups="Broken")
+    @Test
     public void testAbstractEnricherDeduplicationBroken() {
         final TestApplication app = mgmt.getEntityManager().createEntity(EntitySpec.create(TestApplication.class)
                 .enricher(EnricherSpec.create(EmittingEnricher.class)));


Mime
View raw message