commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1598071 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs: auxiliary/disk/ engine/control/ engine/memory/ utils/logger/ utils/struct/
Date Sun, 01 Jun 2014 22:43:10 GMT
On 1 June 2014 20:19, Romain Manni-Bucau <rmannibucau@gmail.com> wrote:
> well it is for sure thread safe. Not sure I get why final and synch would be
> mandatory in this particular case (field will maybe be cached by thread but
> that's not an issue since the value will be unique).

non-final fields are not guaranteed to be published across threads in
the absence of sync.

The LogHelper class has two such fields and is therefore not thread-safe.

> I have nothing against a revert/reapply. I'll open a thread on logging btw.

Thanks.

>
> Romain Manni-Bucau
> Twitter: @rmannibucau
> Blog: http://rmannibucau.wordpress.com/
> LinkedIn: http://fr.linkedin.com/in/rmannibucau
> Github: https://github.com/rmannibucau
>
>
> 2014-06-01 20:55 GMT+02:00 sebb <sebbaz@gmail.com>:
>
>> On 1 June 2014 18:54, Romain Manni-Bucau <rmannibucau@gmail.com> wrote:
>> >
>> > 2014-06-01 19:45 GMT+02:00 sebb <sebbaz@gmail.com>:
>> >>
>> >> PING
>> >>
>> >
>> > Pong, sorry, missed this one.
>> >
>> >>
>> >> On 29 May 2014 03:00, sebb <sebbaz@gmail.com> wrote:
>> >> > On 28 May 2014 18:06,  <rmannibucau@apache.org> wrote:
>> >> >> Author: rmannibucau
>> >> >> Date: Wed May 28 17:06:12 2014
>> >> >> New Revision: 1598071
>> >> >>
>> >> >> URL: http://svn.apache.org/r1598071
>> >> >> Log:
>> >> >> using reentrant locks instead of old synchronized
>> >> >
>> >> > -1
>> >> >
>> >> > This commit mixes two completely different changes:
>> >> > - re-entrant locks
>> >> > - addition of LogHelper
>> >> >
>> >> > I think the LogHelper class is a bad idea. It is also not
>> >> > thread-safe.
>> >> > If the cache is enabled, then it is not possible to change the
>> >> > logging
>> >> > level during a run.
>> >> >
>> >
>> >
>> > How can it be not thread safe? (it is a constant)
>>
>> The class is not thread-safe as it has a non-final non-synch variable
>>
>> > You can disable caching. The main point is jcs tests a lot log debug
>> > level
>> > and depending your backing impl it can be slow (JUL, Log4j 1.2 for what
>> > I
>> > tested). There is a property if you want to disable this feature but
>> > actually in prod you rarely do it.
>> >
>>
>> I have two objections to the commit.
>>
>> 1) the commit mixes two completely different changes
>>
>> 2) the LogHelper class is a bad idea, as it prevents changing the logging
>> level.
>>
>> I suggest reverting the commit and re-applying the locking change on its
>> own.
>> The logging change really needs to be discussed first.
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message