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 14:24:06 GMT

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



server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java
<https://reviews.apache.org/r/22685/#comment82806>

    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.



server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java
<https://reviews.apache.org/r/22685/#comment82809>

    Is there any problem with these being long lived, e.g. in case a particular table is deleted
and then recreated with different configuration?



server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java
<https://reviews.apache.org/r/22685/#comment82807>

    can you add a javadoc about what cache is getting invalidated?



server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfiguration.java
<https://reviews.apache.org/r/22685/#comment82810>

    nit: whitespace



server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfigurationFactory.java
<https://reviews.apache.org/r/22685/#comment82811>

    nit: final



server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfigurationFactory.java
<https://reviews.apache.org/r/22685/#comment82812>

    nit: call out which params may be null (and any special behavior caused by null)



server/base/src/test/java/org/apache/accumulo/server/conf/NamespaceConfigurationTest.java
<https://reviews.apache.org/r/22685/#comment82813>

    can you format these to put @Before and @Test on the line prior ot the method signatutre?



server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java
<https://reviews.apache.org/r/22685/#comment82814>

    Can you edit htis to put @Before, @BeforeClass, and @Test on the line prior to method
signature?



server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java
<https://reviews.apache.org/r/22685/#comment82816>

    Can you edit these to put @Before and @Test on the line prior to method declaration?



server/base/src/test/java/org/apache/accumulo/server/conf/ZooCachePropertyAccessorTest.java
<https://reviews.apache.org/r/22685/#comment82817>

    use Constants.UTF8?


- Sean Busbey


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