Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============4933369156246492439=="
MIME-Version: 1.0
Subject: Re: Review Request 22658: ACCUMULO-2889: Batch metadata updates for
 new WALs
From: "Jonathan Park" <parkjsung@gmail.com>
To: "Jonathan Park" <parkjsung@gmail.com>, "Mike Drob" <mdrob@mdrob.com>,
 "accumulo" <dev@accumulo.apache.org>, "Josh Elser" <josh.elser@gmail.com>
Date: Tue, 17 Jun 2014 05:36:15 -0000
Message-ID: <20140617053615.6312.38177@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Jonathan Park" <noreply@reviews.apache.org>
References: <20140617035545.13794.50266@reviews.apache.org>
In-Reply-To: <20140617035545.13794.50266@reviews.apache.org>
Reply-To: "Jonathan Park" <parkjsung@gmail.com>

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


> On June 17, 2014, 3:55 a.m., Mike Drob wrote:
> >

Thanks for the review!


> On June 17, 2014, 3:55 a.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java, lines 122-123
> > <https://reviews.apache.org/r/22658/diff/1/?file=610893#file610893line122>
> >
> >     If this was not static then you wouldn't need to pass instance, credentials, or configuration. Could be easier to unit test.

Good point, I'll remove the static.


> On June 17, 2014, 3:55 a.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java, line 165
> > <https://reviews.apache.org/r/22658/diff/1/?file=610893#file610893line165>
> >
> >     Lifecycle management: what happens if a user forgets to call commit? Maybe JavaDoc that it is important.
> 
> Josh Elser wrote:
>     s/user/server/ ?
> 
> Josh Elser wrote:
>     Sorry, I didn't realize this was in client. This is in impl though so it's not "user" facing.

Yes, this is a good point. So, I'm a little uneasy with having a thread readily publish to some global state. What are your thoughts on having a fresh Writer instance for every batch of metadata updates? It'll let us avoid having to worry about lifecycle management.

If this is okay as is, I'll add the JavaDoc for internal use.


> On June 17, 2014, 3:55 a.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java, lines 226-228
> > <https://reviews.apache.org/r/22658/diff/1/?file=610893#file610893line226>
> >
> >     Why is this in a finally block? It is only true if the try block finished normally, so we can just call it.

Good catch, I'll remove it from the finally.


> On June 17, 2014, 3:55 a.m., Mike Drob wrote:
> > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java, lines 139-150
> > <https://reviews.apache.org/r/22658/diff/1/?file=610894#file610894line139>
> >
> >     Infinite retry makes me uneasy. Especially with no back-off in between.
> 
> Josh Elser wrote:
>     This is pretty typical. If the master is dead, we're already dead in the water. We want to keep trying until we get a success.

I mostly just went with what other methods in MetadataTableUtil were doing (namely update()). I wouldn't mind replacing the infinite retry with a conditional one. What do we do in the case that we can't succeed? I would imagine we would have to propagate an exception up the stack and inform the client.


> On June 17, 2014, 3:55 a.m., Mike Drob wrote:
> > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java, lines 493-512
> > <https://reviews.apache.org/r/22658/diff/1/?file=610894#file610894line493>
> >
> >     Could be a single method? Implementations look very similar.

Agreed it could, but then we would need a flag that indicates 1 route over another. I wanted to avoid that (though I might be making a problem out of nothing). What are your thoughts?


> On June 17, 2014, 3:55 a.m., Mike Drob wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java, lines 260-284
> > <https://reviews.apache.org/r/22658/diff/1/?file=610897#file610897line260>
> >
> >     What is the danger of holding on to locks for a long time here? Is it possible that other batches will be forced to wait a long time?

Yes, it is possible for other batches to be forced to wait a long time. A similar problem existed before but batching increased the scope of the locks since now all members of a batch are "in it together". If the locks are held for too long, other updates and minor compactions that occur on any of the tablets in a batch will be affected (before it would've just affected the one tablet currently updating). 

As a result, there are a few worrying edge cases here such as extremely large batches (pointed out by Adam). We can mitigate this by limiting the size of each batch (possibly making this configurable with a configuration of size 1 => original behavior).


- Jonathan


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


On June 16, 2014, 9:59 p.m., Jonathan Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22658/
> -----------------------------------------------------------
> 
> (Updated June 16, 2014, 9:59 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2889
>     https://issues.apache.org/jira/browse/ACCUMULO-2889
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> Added additional methods to the Writer class to handle batching. 
> 
> Potential risks are that we're now holding onto the locks for a bit longer than we used to. All tablets present in a batch will have their logLock's locked until the batch is complete. 
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7 
>   server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java 374017d 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java 36b2289 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java e2510d7 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java d25ee75 
>   test/src/test/java/org/apache/accumulo/test/BatchMetadataUpdatesIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22658/diff/
> 
> 
> Testing
> -------
> 
> Added a new IT.
> 
> We insert a few entries to tablets and ensure that the relevant entries appear in the WAL and Metadata Table.
> One test case that isn't included yet is verifying that root + metadata table entries are entered correctly.
> 
> 
> Thanks,
> 
> Jonathan Park
> 
>


--===============4933369156246492439==--