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