> On July 2, 2014, 10:24 a.m., Sean Busbey wrote: > > server/base/src/test/java/org/apache/accumulo/server/conf/ZooCachePropertyAccessorTest.java, > > line 45 > > <https://reviews.apache.org/r/22685/diff/2/?file=616153#file616153line45> > > > > use Constants.UTF8?
Yes, will do. It'll be StandardCharsets when this gets merged to master. > On July 2, 2014, 10:24 a.m., Sean Busbey wrote: > > server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java, > > line 58 > > <https://reviews.apache.org/r/22685/diff/2/?file=616152#file616152line58> > > > > Can you edit these to put @Before and @Test on the line prior to method > > declaration? I neglected to format this code to our standards, so I'll do that and this will be fixed. Ugh, I missed three of them. :) > On July 2, 2014, 10:24 a.m., Sean Busbey wrote: > > server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java, > > line 61 > > <https://reviews.apache.org/r/22685/diff/2/?file=616151#file616151line61> > > > > Can you edit htis to put @Before, @BeforeClass, and @Test on the line > > prior to method signature? I neglected to format this code to our standards, so I'll do that and this will be fixed. > On July 2, 2014, 10:24 a.m., Sean Busbey wrote: > > server/base/src/test/java/org/apache/accumulo/server/conf/NamespaceConfigurationTest.java, > > line 58 > > <https://reviews.apache.org/r/22685/diff/2/?file=616150#file616150line58> > > > > can you format these to put @Before and @Test on the line prior ot the > > method signatutre? I neglected to format this code to our standards, so I'll do that and this will be fixed. > On July 2, 2014, 10:24 a.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java, > > lines 171-179 > > <https://reviews.apache.org/r/22685/diff/2/?file=616145#file616145line171> > > > > can you add a javadoc about what cache is getting invalidated? Sure. I'll do the same for NamespaceConfiguration. > On July 2, 2014, 10:24 a.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java, > > lines 69-70 > > <https://reviews.apache.org/r/22685/diff/2/?file=616145#file616145line69> > > > > Is there any problem with these being long lived, e.g. in case a > > particular table is deleted and then recreated with different configuration? I don't believe so. Here's some background info. - The ZooCachePropertyAccessor object is really just a wrapper around a ZooCache and contains the logic for finding namespace-specific or table-specific ZK data in the ZooCache. So the key is whether the ZooCache stays valid. - A watcher is set when the ZooCache is initially created. The watcher pays attention to changes under the ZK node that holds configuration data for the table. I expect that if a table is deleted, then its configuration data is removed from ZK and so the watcher will be notified and update the cache. Alternatively, if a table is re-created, then the data would be flushed and/or reinitialized, and again the watcher would find out. So I'm relying on the ZooCache to work correctly. > On July 2, 2014, 10:24 a.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java, > > lines 66-76 > > <https://reviews.apache.org/r/22685/diff/2/?file=616145#file616145line66> > > > > Is the additional synchronized block needed here? > > > > The method is already synchronized, the member is private, and I don't > > see it used anywhere outside of the method. The getPropCacheAccessor() method (and the invalidateCache() method too) synchronize on the class instance for thread-safe access to the propCacheAccessor field, so that the field is only initialized once. The synchronized block inside getPropCacheAccessor guards access to the static propCaches map, across all class instances, so that two TableConfiguration objects won't create two accessor objects for the same table. So the synchronizations are for separate reasons. At least, that's my take on it. Definitely check if that makes sense. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22685/#review47187 ----------------------------------------------------------- On June 27, 2014, 1:11 p.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22685/ > ----------------------------------------------------------- > > (Updated June 27, 2014, 1:11 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2615 > https://issues.apache.org/jira/browse/ACCUMULO-2615 > > > Repository: accumulo > > > Description > ------- > > What started out as a simplification effort on server configurations ended up > with results that aren't so much simpler but are hopefully more > straightforward. Here is a summary of the more important changes. > > * ServerConfiguration is deprecated in favor of the new > ServerConfigurationFactory. > * ServerConfigurationFactory caches configurations not just but namespace or > table ID, but also by instance ID. > * ZooConfigurationFactory takes over the creation logic from > ZooConfiguration. A ZooConfiguration instance is no longer a singleton, but > an ordinary instance. ZooConfigurationFactory caches created ones by instance > ID. > * NamespaceConfiguration and TableConfiguration each keep a static map of > ZooCache instances, rather than a single static instance. The map is keyed by > instance ID and namespace/table (resp.) ID. > * getParentConfiguration() was added to all appropriate configurations. (I > did have an interface declaring the method but decided it was not useful.) > * The logic for retrieving properties from ZooCache instances is refactored > to a new ZooCachePropertyAccessor object. > * NamespaceConfWatcher and TableConfWatcher include the configurations they > watch as fields, so they no longer need to repeatedly look them up (formerly > in ServerConfiguration). As a consequence, the watchers no longer attempt to > handle received WatchedEvents for a different namespace or table. (This > *should* never happen - a warning is logged if it does.) > > > Diffs > ----- > > > server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfWatcher.java > 50ab27ee8dcab41d89789ea409f5e554c56163d3 > > server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfiguration.java > ebb098b01798bd4478f7d487909409bd2ca2baf1 > > server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfiguration.java > 2115f3f28557f1a56b4e117a5ce4e0be920a09d7 > > server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfWatcher.java > 3c24ec49aa4895283e6556b02ed3479dda560b6b > > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java > b538d2ecc16a0c4c0456d491afb5e116ae0000b5 > > server/base/src/main/java/org/apache/accumulo/server/conf/TableParentConfiguration.java > 34eb781a5b2807e3457a5f6ac501088ac2fc2e7a > > server/base/src/main/java/org/apache/accumulo/server/conf/ZooCachePropertyAccessor.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfiguration.java > 696873bb013946c670c9d781cdf43bb0cb143b98 > > server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfigurationFactory.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/conf/NamespaceConfigurationTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java > ae73ba69a87bf40de9ec8c3b8f804bb475a810ef > > server/base/src/test/java/org/apache/accumulo/server/conf/ZooCachePropertyAccessorTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/conf/ZooConfigurationFactoryTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/22685/diff/ > > > Testing > ------- > > Unit tests pass. Basic exercising on a one-node cluster (continuous ingest) > successful. All integration tests pass (after ACCUMULO-2951 and related). > > > Thanks, > > Bill Havanki > >
