Another strange stacktrace in agent log

Jay Shaughnessy jshaughn at redhat.com
Tue Feb 11 17:17:05 UTC 2014


Hi Thomas,

OK, sorry, I wasn't aware that we were already in this situation given 
Ian's work to address the concurrency issue at an earlier time.   
Although I realize that the CopyOnWriteArraySet implementation may not 
be necessary on the server-side the benefit gained is a simpler 
implementation overall.   And, we have seen instances of 
ConcurrentModificationException server-side, although I'm not sure it 
has affected Resource children.   I'm not sure we'll give up too much 
server-side as I think most of the childResources setting is done once, 
and child cardinality tends to be small.  Even if we took a hit in some 
areas of the code, I doubt it would be noticeable, in the overall scheme 
of things.

The agent-side gets implicit thread-safety and would not need any 
changes/special case code.

One thing I see in the branched impl is that we don't allow a null for 
childResources, when the custom Set is in place, the remove() method 
ensures an empty Set.  This may be OK but I think Heiko tried hard to 
ensure we don't have empty collections lying around agent-side.

Anyway, I can go either way, but I'm attracted to the idea of a single 
Set impl for resource children.  Moreover, I think there are other Sets 
that could benefit.  A search of the server code shows several places 
where we copy child sets to avoid ConcurrentModExceptions (like 
childResourceTypes).


On 2/11/2014 3:57 AM, Thomas Segismont wrote:
> Le 10/02/2014 22:13, Jay Shaughnessy a écrit :
>>
>> I guess I still don't quite get the details here.   But first one basic
>> up-front question:  would we want to consider just making
>> Resource.childResources always be a CopyOnWriteArraySet?  Or is the
>> thinking that it would incur unnecessary overhead server-side?
>
> On the server side it's not really useful: Resource objects are JPA 
> entities and should (almost) always not be updated concurrently.
>
>>
>> More questions inline:
>>
>> On 2/10/2014 12:12 PM, Thomas Segismont wrote:
>>> Le 10/02/2014 17:34, Jay Shaughnessy a écrit :
>>>> A couple of notes/questions on the current branch changes:
>>>> - I'm not sure I quite understand the changes to Resource, with the
>>>> customChildResourcesCollection flag. is this really necessary?
>>>
>>> It is necessary otherwise you can't guarantee that all threads are
>>> working on the same, thread-safe collection.
>>
>> I guess I'm not sure I like the idea of the Resource entity behaving
>> differently based on the constructor being used.  What exactly makes
>> this necessary?  Why can't we, agent-side, just ensure the
>> childResources Set is a CopyOnWriteArraySet, converting when the
>> information comes from the server, or initializing when created on the
>> agent?
>
> I wasn't in a very creative mood so I just put the code back in the 
> state it was before commit 5a63da5b :)
>
> Jokes aside, the Resource class constructor was introduced by Ian in 
> commit 3e149ee on 02/21/2012. Have a look at the commit:
> https://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?id=3e149ee
> The purpose was exactly what we're talking about: "Use a 
> ConcurrentHashMap-based Set for childResources to allow the field to 
> be concurrently accessed safely"
>
> The only difference I added is the use of CopyOnWriteArraySet instead 
> of the default-initialized ConcurrentHashMap-based Set. 
> Default-initialized CHM are very bad from a memory consumption 
> perspective when you expect a few or no elements in the collection and 
> a low number of concurrent threads updating the collection:
> http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html#ConcurrentHashMap%28%29 
>
> "default initial capacity (16), load factor (0.75) and 
> concurrencyLevel (16)"
>
>>
>> Or, as asked above, maybe just settle on the thread-safe type and be
>> done with it.
>>
>
> That would be a step backwards considering the job done in commit 
> 5a63da5b. But if everyone is happy with a simpler version and we still 
> meet the performance expectations, I'm fine.
>
> Thomas
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel



More information about the rhq-devel mailing list