commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@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 19:19:39 GMT
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).

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



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.
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message