Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============0508999020115962150=="
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>,
 "accumulo" <dev@accumulo.apache.org>,
 keith@deenlo.com
Date: Mon, 30 Jun 2014 16:00:46 -0000
Message-ID: <20140630160046.5196.55181@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Jonathan Park" <noreply@reviews.apache.org>
References: <20140630152404.5195.26737@reviews.apache.org>
In-Reply-To: <20140630152404.5195.26737@reviews.apache.org>
Reply-To: "Jonathan Park" <parkjsung@gmail.com>

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


> On June 30, 2014, 3:24 p.m., kturner wrote:
> >

Thanks for the review!


> On June 30, 2014, 3:24 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java, line 261
> > <https://reviews.apache.org/r/22658/diff/2/?file=619604#file619604line261>
> >
> >     Rather than having the thread local, it would be cleaner to have a list of mutations and populate it in the loop.  At the end of the loop pass the list of mutations to method that writes it.

Ah, I like this suggestion and will work towards getting it in. I was stumped on how to properly resolve the use of a ThreadLocal for a while. Thanks!


> On June 30, 2014, 3:24 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java, line 262
> > <https://reviews.apache.org/r/22658/diff/2/?file=619604#file619604line262>
> >
> >     Maybe this is a problem in the existing code, but this change may make the problem worse.  Calling beginUpdatingLogsUsed() updates the tablets in memory data structs to indicate the tablet is using the log.  If the code after this point fails, then all tablets will think the walog is in the metadata table even though it is not.  
> >     
> >

I guess you're suggesting that if we failed for whatever reason while holding the lock, our changes shouldn't be published outside of the lock (since all readers of the wal in memory structs also lock).

I think we could either implement some kind of rollback or convince ourselves that it's harmless (or implement it so that it is harmless). I'll give this some thought.


> On June 30, 2014, 3:24 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java, line 267
> > <https://reviews.apache.org/r/22658/diff/2/?file=619604#file619604line267>
> >
> >     would be interesting to see performance diff w/ this change and ACCUMULO-2801
> >

Bump on the comments on that ticket :)


> On June 30, 2014, 3:24 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java, line 270
> > <https://reviews.apache.org/r/22658/diff/2/?file=619604#file619604line270>
> >
> >     could modify this method to construct a mutation and return it.  The returned mutation could be added to the list.

Agreed. It looks like we don't benefit much from having the batching implementation hidden away in another class and would benefit from batching the changes ourselves.


- Jonathan


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


On June 27, 2014, 5:41 p.m., Jonathan Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22658/
> -----------------------------------------------------------
> 
> (Updated June 27, 2014, 5:41 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/InternalBatchWriter.java PRE-CREATION 
>   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 57415bd 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java d25ee75 
>   test/src/test/java/org/apache/accumulo/server/util/InternalBatchWriterIT.java PRE-CREATION 
>   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
> 
>


--===============0508999020115962150==--