accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 28873: ACCUMULO-3393 Follow-on work for per-table volume chooser.
Date Wed, 10 Dec 2014 07:54:50 GMT

-----------------------------------------------------------
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
> 
>


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