----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28873/#review64511 -----------------------------------------------------------
Overall, I saw some good modifications. However, some of the changes seemed to add unnecessary (premature) performance optimizations (object pool for a singleton map in an infrequently called method, set intersection with arrays instead of java collections). The volume chooser is an infrequently called method that represents a relatively trivial amount of the cost of creating a new tablet (adding a split point). I'm not sure that warrants the additional dependency or code complexity of those optimizations. And some of them, I'm just not confident by looking at them that they do the same thing as before (the String array set intersection part), when what was there occurred in two lines. I do like the lazy initialization for the ServerConfigurationFactory, and the experimental annotation on the per-table property, and the javadoc updates. Overall, though, it is very difficult to review this in the context of whether or not it satisfies the goals of ACCUMULO-3393, since those goals are not described in the JIRA issue. The changes here do not appear to be "clean-up" in any sense of the phrase I'm familiar with, since the changes do not appear to result in "cleaner" code. If anything, some of the changes are more complex and cumbersome to read than before, without changing their behavior or adding any measurable performance gain to tablet creation. I'm also concerned that the change in behavior which adds burdensome restrictions to the ability to dynamically change controlling per-table properties is not a "clean-up", either. These changes are mixed in with a few documentation related changes and some changes I think are beneficial (above), so I don't want to dismiss the whole thing as negative, but I do think that this change tends to introduce more problems than it solves, especially since the problems it intends to solve are not defined. core/src/main/java/org/apache/accumulo/core/conf/Property.java <https://reviews.apache.org/r/28873/#comment107249> +1 server/base/pom.xml <https://reviews.apache.org/r/28873/#comment107250> New dependency? Does it add significant value to warrant additional dependency conflict resolution and packaging complexity? server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107254> 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). server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107278> 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. server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107276> This is incorrect. This is not an experimental property. It is a custom per-table property, specific to the PreferredVolumeChooser. server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107257> 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. server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107258> nit: formatting/whitespace server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107277> 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. server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107259> How is this implementation better than the simple, readable, higher-level collections-based implementation? server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java <https://reviews.apache.org/r/28873/#comment107260> If you get here, the modifiedOptions had better not be null. Shouldn't it have numValid entries in it? server/base/src/main/java/org/apache/accumulo/server/fs/VolumeChooser.java <https://reviews.apache.org/r/28873/#comment107263> N.B.? server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java <https://reviews.apache.org/r/28873/#comment107269> No good reason. This was brought in with the changes I proposed to Jenna, to use the existing utility method for instantiation. This should be the property default (which is now the per-table chooser). server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java <https://reviews.apache.org/r/28873/#comment107268> +1 for this check, but this should be a separate issue, and also would benefit 1.6 as a sanity-check/bug-prevention measure. server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java <https://reviews.apache.org/r/28873/#comment107270> Was this a formatting-only change? server/base/src/main/java/org/apache/accumulo/server/util/TabletOperations.java <https://reviews.apache.org/r/28873/#comment107274> Note: I've been playing with the Eclipse plugin UCDetector, which is pretty good at finding unused code. Might be good for us to run that (or equiv) for QC before next major release to catch all these unused non-public API leftovers. - Christopher Tubbs On Dec. 10, 2014, 1:01 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, 1:01 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 > * 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 > >
