[GitHub] incubator-brooklyn pull request: Fix concurrent use of entity.setA...

[aledsage]

Github user aledsage commented on the pull request:


https://github.com/apache/incubator-brooklyn/pull/1110#issuecomment-165446246
  
@ahgittin thanks. Interesting we weren't getting any 
`ConcurrentModificationException`s that I noticed. Instead we just had some 
subset of the 100 setAttribute calls actually having worked! I encountered this 
when writing tests for 
https://github.com/brooklyncentral/advanced-networking/pull/86. It was most 
confusing and frustrating when assuming the bug was in advanced-networking, 
until I left the advanced-networking and tried a simple tests in brooklyn!

@sjcorbett thanks; will update that javadoc and then merge.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Fix concurrent use of entity.setA...

[sjcorbett]

Github user sjcorbett commented on a diff in the pull request:

https://github.com/apache/incubator-brooklyn/pull/1110#discussion_r47898623
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/sensor/AttributeMap.java ---
@@ -59,7 +60,20 @@
  * Creates a new AttributeMap.
  *
  * @param entity the EntityLocal this AttributeMap belongs to.
- * @throws IllegalArgumentException if entity is null
+ * @throws NullPointerException if entity is null
+ */
+public AttributeMap(AbstractEntity entity) {
+// Not using ConcurrentMap, because want to (continue to) allow 
null values.
+// Could use ConcurrentMapAcceptingNullVals (with the associated 
performance hit on entrySet() etc).
+this(entity, Collections.synchronizedMap(Maps.newLinkedHashMap()));
+}
+
+/**
+ * Creates a new AttributeMap.
+ *
+ * @param entity the EntityLocal this AttributeMap belongs to.
+ * @param values the Map in which to store the values - should be 
concurrent or synchronized.
--- End diff --

values should be storage.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Fix concurrent use of entity.setA...

[aledsage]

GitHub user aledsage opened a pull request:

https://github.com/apache/incubator-brooklyn/pull/1110

Fix concurrent use of entity.setAttribute()

Previously we were using a vanilla LinkedHashMap for storing attributes
(ever since the FEATURE_USE_BROOKLYN_LIVE_OBJECTS_DATAGRID_STORAGE
was disabled by default!)

Now uses synchronized set / synchronized map (rather than `ConcurrentMap`, 
as need to accept null values), with underlying `LinkedHashSet` to preserve 
order.

Adds `EntityConcurrencyTest` for concurrently:
- setting attributes (previously failed fairly often)
- setting config
- adding tags
- adding groups (previously failed sometimes)
- adding children
- adding locations
- adding policies
- adding enrichers
- adding feeds

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/aledsage/incubator-brooklyn 
fix/concurrent-setAttribute

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-brooklyn/pull/1110.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1110


commit b9f8dc1fc1ea9527705bc8ce1b54e66bfaaa9815
Author: Aled Sage 
Date:   2015-12-15T21:41:29Z

Fix concurrent use of entity.setAttribute()

Previously we were using a vanilla LinkedHashMap for storing attributes
(ever since the FEATURE_USE_BROOKLYN_LIVE_OBJECTS_DATAGRID_STORAGE
was disabled by default!)

Now uses ConcurrentMap, or for some other things a synchronised set
so that we preserve order with the underlying LinkedHashSet.

Adds EntityConcurrencyTest for concurrently:
- setting attributes
- setting config
- adding tags
- adding groups
- adding children
- adding locations
- adding policies
- adding enrichers
- adding feeds




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---