accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Busbey" <s...@manvsbeard.com>
Subject Re: Review Request 22685: ACCUMULO-2615 - refactored server configurations
Date Wed, 02 Jul 2014 15:44:15 GMT


> On July 2, 2014, 2:24 p.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.
> 
> Bill Havanki wrote:
>     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.

ah. missed the static on propCaches.

LGTM.


- Sean


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


On June 27, 2014, 5: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, 5: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
> 
>


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