[GitHub] incubator-brooklyn pull request: Fix concurrent use of entity.setA...
Github user ahgittin commented on the pull request: https://github.com/apache/incubator-brooklyn/pull/1110#issuecomment-165462742 @aledsage yikes -- great this is fixed! --- 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...
Github user asfgit closed the pull request at: https://github.com/apache/incubator-brooklyn/pull/1110 --- 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...
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...
Github user sjcorbett commented on the pull request: https://github.com/apache/incubator-brooklyn/pull/1110#issuecomment-165437080 +1. I've left one trivial comment but otherwise looks good to me. --- 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...
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., Object>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...
Github user ahgittin commented on the pull request: https://github.com/apache/incubator-brooklyn/pull/1110#issuecomment-165433735 looks good. presumably the objective is to prevent concurrent mod exceptions, and caller is still not granted access to the concurrency model. so you can't e.g. *increment* a sensor. but this would allow us to add a `modify(Sensor, Function modification)` method which would modify atomically. --- 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...
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. ---