accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 28873: ACCUMULO-3393 Follow-on work for per-table volume chooser.
Date Wed, 10 Dec 2014 20:11:09 GMT


> On Dec. 10, 2014, 2:54 a.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java,
line 39
> > <https://reviews.apache.org/r/28873/diff/1/?file=787888#file787888line39>
> >
> >     The volatile for lazy-initialization is not needed. We don't suffer from having
this assigned twice, which this code doesn't prevent. Also, it should be initialized to null
here before it is assigned to localConf.
> >     
> >     It should also be ServerConfigurationFactory. The ServerConfiguration type was
only retained internally after Havanki's refactor to avoid changing the balancer API.
> >     
> >     Ideally, we'd have an init method and be able to pass in the AccumuloServerContext,
but this was problematic last I looked, because it created a dependency cycle between the
VolumeManager and ServerConfigurationFactory, because this is called by the VolumeManager
code and that is needed by the ServerConfigurationFactory (so it can't depend on the ServerConfigurationFactory).
> 
> Sean Busbey wrote:
>     The volatile is needed to ensure that Object initialization is properly handled before
the Object is visible to other threads.
>     
>     The default value is already null, what's the advantage of repeating it?
>     
>     If ServerConfiguration isn't intended to be used as a placeholder for the more general
case, can you please file a follow on to mark it @Deprecated with a note?
>     
>     The TODO above the variable calls out the preference for an init and the reason it
can't currently happen. Will filing that ticketa address that part of your comment?
> 
> Christopher Tubbs wrote:
>     The object (ServerConfigurationFactory) is fully initialized before it is assigned.
I'm not sure what you're referring to. The use of volatile in this context can help (in newer
JVMs, certainly in 1.6 and later) in the double-check idiom for lazy initialization. However,
this is not the idiom being employed here. Nothing in this code prevents the object from being
assigned twice (by another thread), which is what the double-check idiom is for. All this
volatile does is prevent the JVM from caching the current reference.
>     
>     The suggestion for null is just a nice to have. It makes it clear what the default
value is. It's not important.
>     
>     ServerConfiguration predates the switch to ServerConfigurationFactory. Yes, more
follow-on work needs to be done to remove it completely. That work was discussed and deferred
in https://issues.apache.org/jira/browse/ACCUMULO-2615 and its sub-tasks. In the meantime,
I'm simply suggesting to avoid it, as the factory more explicitly conveys its function (it
is used *as* a factory, not as a configuration).
>     
>     I'm not too concerned about the timeline for adding an init method. If you want to
create a JIRA, go ahead. I'd have to put more thought into that. The main point of that comment
was to empathize/agree that an init method might be helpful. On the other hand, if we don't
guarantee that we can maintain state, it may not be any better than passing stuff in with
the environment object on the choose method.
> 
> Sean Busbey wrote:
>     The specific technique being employed is named the "single-check idiom" and is for
the case where you want to avoid lots of initialization, but can stomach some repeated initialization.
You are correct that it does not prevent multiple assignments. This is because all it needs
to work is ensure that each of those assignments can happend safely in a multithreaded context.
>     
>     The JVM and the compiler are allowed to delay Object initialization. If the variable
isn't volatile then the combination of compiler and jvm optimiazations can result in one of
the assignemnts happening prior to the object finishing (or even starting) initialization.
That means a different thread might get to the initial null check after the assignment but
prior to initialization happening in one of the threads doing initialization. That other thread
can then see incorrect values in fields present on the object.
>     
>     Incidentally, this is the same reason a proper double-check idiom needs the variable
to be volatile. Because in that case even though you've reduced the number of potentially
incorrect assignment interacitons to 1, it can still happen. This is also why the double-check
idiom did not work prior to JDK5 (because volatile did not cause the same memory boundary
conditions).
> 
> Christopher Tubbs wrote:
>     Okay, that makes sense, but it seems to also imply that no fields are thread-safe
unless they are volatile. For my own development, can you cite a source for your information,
so I can read up?
>     At the very least, would not it make more sense to just use AtomicReference for simpler
readability with the same guarantees?

Actually, nevermind on that citation. A colleague explained to me that the JVM can reorder
assignments in the Object's constructor to after the object's own reference is assigned, and
also how volatile protects against this. So, it makes sense to me now. However, I think AtomicReference
might still improve readability for people not familiar with the intricacies of Java's memory
model and the side-effects of volatile.


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28873/#review64511
-----------------------------------------------------------


On Dec. 10, 2014, 9:58 a.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28873/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2014, 9:58 a.m.)
> 
> 
> Review request for accumulo and Jenna Huston.
> 
> 
> Bugs: ACCUMULO-3393
>     https://issues.apache.org/jira/browse/ACCUMULO-3393
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3393 Follow-on work for per-table volume chooser.
> 
> Still has TODOs for additional tickets I need to file. I'll update the review to remove
once I have them all filed. I think everything marked TODO can wait for a later ticket. Please
comment on relevant section if you think something needs to be done now.
> 
> * docs clean up + code guideline compliance.
> * ensure RandomVolumeChoosers are independent when used per-table.
> * make sure that per-table choosers can keep state the way that global choosers can
> * make sure that a chooser can only pick from the options it is presented.
> * avoid object creation in critical path for Tablet.split.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java 4c2d0b41b3bce32449861da3ac42fa27deb2b182

>   pom.xml 31601a1bf84e19b861e4f48b50824eeb77987b52 
>   server/base/pom.xml c21a168dec2092692f1ee6877c4703ee2d3e3977 
>   server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java
7a825c796eb5a25de8c748e2aba642f483697b9a 
>   server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java
7ed7bba809a4e5e4b2d472c3327b15adb37251a7 
>   server/base/src/main/java/org/apache/accumulo/server/fs/RandomVolumeChooser.java f2eb2113cb848ed58ac5f41573c6ff2cde9b0a77

>   server/base/src/main/java/org/apache/accumulo/server/fs/VolumeChooser.java f523057b11a2dc42e82010675bb1ac8e3802f96d

>   server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManager.java 890651e92f4c34514cb839b7b9ee9d23ad55070a

>   server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java dc1be739b634d91992894f8f27c2d9c184bd98cd

>   server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java 877d01c2233cca086c1ac1539eb81cc282a7ceb4

>   server/base/src/main/java/org/apache/accumulo/server/util/TabletOperations.java c0e1a9b991d61a4dbb127c74ae297f171434e7d5

>   server/base/src/test/java/org/apache/accumulo/server/fs/VolumeManagerImplTest.java
582822a8ccbc398925a2184eed0b9d7fa853f9b4 
> 
> Diff: https://reviews.apache.org/r/28873/diff/
> 
> 
> Testing
> -------
> 
> Ran through VolumeManagerImplTest and VolumeChooserIT 
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>


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