Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============2609208680235622865=="
MIME-Version: 1.0
Subject: Re: Review Request 28873: ACCUMULO-3393 Follow-on work for per-table
 volume chooser.
From: "Sean Busbey" <sean@manvsbeard.com>
To: "Jenna Huston" <jenna.huston22@gmail.com>
Cc: "Mike Drob" <mdrob@mdrob.com>, "Sean Busbey" <sean@manvsbeard.com>,
 "accumulo" <dev@accumulo.apache.org>,
 "Christopher Tubbs" <ctubbsii-rb@apache.org>,
 "Josh Elser" <josh.elser@gmail.com>
Date: Wed, 10 Dec 2014 16:40:54 -0000
Message-ID: <20141210164054.26419.37309@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Sean Busbey" <noreply@reviews.apache.org>
References: <20141210075450.26419.35187@reviews.apache.org>
In-Reply-To: <20141210075450.26419.35187@reviews.apache.org>
Reply-To: "Sean Busbey" <sean@manvsbeard.com>

--===============2609208680235622865==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, line 44
> > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line44>
> >
> >     This is incorrect. This is not an experimental property. It is a custom per-table property, specific to the PreferredVolumeChooser.
> 
> Sean Busbey wrote:
>     how would you prefer I convey the information that operators can't assume any compatibility guarantees on its use?
> 
> Josh Elser wrote:
>     I don't get this either. The property is defined to take a comma-separated list of Volumes (URIs). If this fact changes, we'd have to make a new property to support whatever we needed it to become. Experimental has been used to define things that are incompletely implemented -- anything else has been handled by documentation.

I'm trying to convey a similar level of reliability as we do with @Experimental on GENERAL_VOLUME_CHOOSER and TABLE_VOLUME_CHOOSER. That is, we might change the variable name or what it holds. Would describing it as a "table property specific to the current implementation of PreferredVolumeChooser" work?


> On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, line 99
> > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line99>
> >
> >     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.
> 
> Sean Busbey wrote:
>     This code path is in the critical section of tablet splitting, the frequency of which varies greatly based on system deployment. There's already a TODO for doing the refactoring you mention, which will allow us to later remove the pooling entirely.
> 
> Josh Elser wrote:
>     I really don't see the creation of a new HashMap as being critical WRT the cost of waiting for a tablet to close, performing multiple metadata table updates and then waiting for the tablets to come back online.

Am I correct that we currently don't have any microbenchmarks that would tell us one way or the other?


> On Dec. 10, 2014, 7:54 a.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java, lines 125-141
> > <https://reviews.apache.org/r/28873/diff/1/?file=787889#file787889line125>
> >
> >     How is this implementation better than the simple, readable, higher-level collections-based implementation?
> 
> Sean Busbey wrote:
>     VC.choose is in the critical path for tablet splitting. The collections based approach does more allocations of much heavier weight objects. Additionally, since all of these things only live in the scope of the method those allocations will cause gc pressure during heavy splitting.
> 
> Josh Elser wrote:
>     bq. will cause gc pressure during heavy splitting.
>     
>     *might* cause gc pressure :). I don't think you can make that assertion without actually quantifying how much "gc pressure" is introduced WRT the amount of time the other known slow operations with comprise splitting a tablet actually take.

Am I correct that we currently don't have any microbenchmarks that would tell us one way or the other?


- Sean


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


On Dec. 10, 2014, 2:58 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28873/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2014, 2:58 p.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 + code guideline compliance.
> * ensure RandomVolumeChoosers are independent when used per-table.
> * 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
> 
>


--===============2609208680235622865==--