> On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/pom.xml, lines 79-82 > > <https://reviews.apache.org/r/28873/diff/1/?file=787887#file787887line79> > > > > New dependency? Does it add significant value to warrant additional > > dependency conflict resolution and packaging complexity?
It's generally better for us to rely on an external dependency that focuses on a task outside of our main concerns since they'll give more focus on things. I didn't see any of our current dependencies providing an object pool. If you know of one, please let me know and I'll switch to it. As for commons-pool in particular, it's a ASF project and it does exactly the task needed and no more. It'll also be removable once the TODO above its use is done. > On Dec. 10, 2014, 7: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). 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? > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java, > > line 53 > > <https://reviews.apache.org/r/28873/diff/1/?file=787888#file787888line53> > > > > This change unnecessarily adds a restriction that the initial volume > > chooser for a table will be fixed for the lifetime of the TabletServer. > > This prevents dynamic changes to a per-table configuration property without > > justification. This is problematic by itself, but especially so given that > > a user is unable to set an initial VolumeChooser for the table because of > > your objection to ACCUMULO-3176. that isn't true. The second if block is used if there already was chooser. In that case it checks to see if the property has changed, using hte same idiom the per table balancer users. > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, > > line 44 > > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line44> > > > > This is incorrect. This is not an experimental property. It is a custom > > per-table property, specific to the PreferredVolumeChooser. how would you prefer I convey the information that operators can't assume any compatibility guarantees on its use? > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, > > line 99 > > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line99> > > > > Do we really need an object pool for a map which holds 0 or 1 element > > for the duration of a single, infrequently called method? A better > > optimization for this case would be to add an optimized get(String key) > > method to AccumuloConfiguration / TableConfiguration, so we don't have to > > use a map at all. This code path is in the critical section of tablet splitting, the frequency of which varies greatly based on system deployment. There's already a TODO for doing the refactoring you mention, which will allow us to later remove the pooling entirely. > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, > > lines 116-122 > > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line116> > > > > This change unnecessarily prevents dynamic changes to preferredVolumes, > > and forces the initial property to be the only one possible to read. I > > would say that this is a problem by itself, but given that you've objected > > to ACCUMULO-3176, which would give users the ability to set initial > > properties, this is even more problematic. This change only caches the parsed version of particular volume strings to avoid calling split on every invocation if the configured volumes haven't changed. How does that prevent the dynamic configuration? > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, > > lines 125-141 > > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line125> > > > > How is this implementation better than the simple, readable, > > higher-level collections-based implementation? VC.choose is in the critical path for tablet splitting. The collections based approach does more allocations of much heavier weight objects. Additionally, since all of these things only live in the scope of the method those allocations will cause gc pressure during heavy splitting. > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, > > line 150 > > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line150> > > > > If you get here, the modifiedOptions had better not be null. Shouldn't > > it have numValid entries in it? If all of the options were valid then we wouldn't have copied anything. ref the comment "avoid copying if a prefix of the options are all fine". Should I expand the comment to explain the optimization? > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeChooser.java, > > line 21 > > <https://reviews.apache.org/r/28873/diff/1/?file=787891#file787891line21> > > > > N.B.? It's a phrase that means the reader should pay attention to the bit that follows. > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java, > > lines 581-586 > > <https://reviews.apache.org/r/28873/diff/1/?file=787893#file787893line581> > > > > +1 for this check, but this should be a separate issue, and also would > > benefit 1.6 as a sanity-check/bug-prevention measure. It's not strictly needed in 1.6 because only someone with control of accumulo-site.xml can set the volume chooser, but I'm happy to file a follow on. > On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java, > > line 269 > > <https://reviews.apache.org/r/28873/diff/1/?file=787894#file787894line269> > > > > Was this a formatting-only change? yes, this was a line that was over 160 columns in the original commits. - Sean ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28873/#review64511 ----------------------------------------------------------- On Dec. 10, 2014, 2:26 p.m., Sean Busbey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28873/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2014, 2:26 p.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 > >
