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 <seb...@gmail.com>: > On 1 June 2014 18:54, Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > > > > 2014-06-01 19:45 GMT+02:00 sebb <seb...@gmail.com>: > >> > >> PING > >> > > > > Pong, sorry, missed this one. > > > >> > >> On 29 May 2014 03:00, sebb <seb...@gmail.com> wrote: > >> > On 28 May 2014 18:06, <rmannibu...@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. >
