hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Todd Lipcon <t...@cloudera.com>
Subject Re: Review Request: HBASE-2578
Date Wed, 26 May 2010 04:10:15 GMT
Why not just initialize it from a static {} block, then in a test if we want
to change it, we change it?

On Tue, May 25, 2010 at 8:54 PM, Ryan Rawson <ryanobjc@gmail.com> wrote:

> Thanks for that salient comment. Perhaps someone can give us the right
> pattern for no lock Singleton initialization.
>
> On May 25, 2010 8:49 PM, "Paul Cowan" <cowan@aconex.com> wrote:
> > On 26/05/2010 1:15 PM, Daniel Ploeg wrote:
> >>>> this is a lot of synchronization for what ends up being on the fast
> path of every query, perhaps there is a non synchronized manner in which we
> can accomplish this? the use of volatile might help here
> >>
> >> thanks Ryan. I think I might use AtomicReference instead - can simplify
> even further this way. I'll upload another patch a bit later.
> >
> > Sorry, no real stake in this review but just a curious third-party: is
> > there any need for even the AtomicReference?
> >
> > Making 'delegate' volatile would completely remove the need for the
> > synchronization in getDelegate(), and the lock on injectEdge(). The only
> > exception is that without synchronization, the null-check-and-default in
> > getDelegate() would be subject to race conditions. Given the desire to
> > reduce synchronization, and the fact that delegate is assigned to
> > default at construction time, I'd humbly suggest that the "if (delegate
> > == null)" be removed from getDelegate(), and instead change injectEdge()
> > to reject null values (or call reset() if you want to keep the effective
> > existing semantics that injectEdge(null) returns to a
> > DefaultEnvironmentEdge).
> >
> > Then the ReentrantLock can be removed, no synchronization is required,
> > and one volatile variable is all that's needed.
> >
> > (Incidentally, I'm not sure what the ReentrantLock is actually FOR --
> > reference assignment is atomic in Java, so it's not protecting against
> > any race conditions; all it could be useful for is a happens-before
> > relationship with the get, but the two are using different locks -- the
> > synchronized get does not have a happens-before relationship with the
> > unlock. Either both should be synchronized, or both use the lock -- as
> > it is, the lock buys you nothing. Mind you with a simple volatile then
> > the happens-before relationship is correct anyway).
> >
> > Cheers,
> >
> > Paul
>



-- 
Todd Lipcon
Software Engineer, Cloudera

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