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