accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 22658: ACCUMULO-2889: Batch metadata updates for new WALs
Date Tue, 17 Jun 2014 05:36:50 GMT


> On June 17, 2014, 12:38 a.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java, line 155
> > <https://reviews.apache.org/r/22658/diff/1/?file=610893#file610893line155>
> >
> >     I'm not quite sure I understand the use of a ThreadLocal here. I'm thinking
about a case where I have multiple tablets on a tserver. I have two writes coming in with
updates to that same tserver, one to each tablet. Aren't those updates going to be running
in their own threads anyways, so we wouldn't actually see any batching happening?
> >     
> >     If I'm missing something, please point me at some code and I'd be happy to try
to understand better myself the context.
> 
> Jonathan Park wrote:
>     So Adam pointed out to me that this code in Writer probably more appropriately belongs
in a server-side class rather than one released to clients. This batching behavior was primarily
designed for internal use within a TabletServer (for now, specifically with metadata table
updates).
>     
>     For some context, the place where this batching is most beneficial is when we're
adding an entry into the Metadata table when a tablet is referenced in a new WAL. Currently,
for all tablets in an "update batch" (startUpdate -> applyUpdates -> closeUpdate), we
make a separate update call one at a time (see TabletServerLogger). In one of our clusters,
we've seen hsync perform unusually slow. Each separate update call will result in 1 hsync
call. If there are many tablets present in an "update batch", this will yield many separate
invocations to hsync. We found our ingest rate dip around the time a WAL rollover occured
as a result.
>     
>     The reasoning behind choosing a ThreadLocal is as follows. I didn't want separate
tserver client threads committing another tserver client's batch of mutations (it may be safe
to do so). Therefore, I wanted some level of isolation provided for each client thread to
simplify the picture. The use of a ThreadLocal could be avoided if each tserver client serving
thread would get its own Writer instance. 2 factors lead me to avoid choosing that route:
>     - I saw that we were typically storing Writer instances and re-using them. 
>     - The use of the Writer class was an implementation detail to the TabletServerLogger.

>     
>     Admittedly, the semantics get trickier as we now need to carefully ensure that the
batch is always emptied when the thread finishes updating new logs (whether it succeeds or
errors).

bq. more appropriately belongs in a server-side class rather than one released to clients

Agreed. I didn't realize you did this in core.client.impl at first.

bq. Currently, for all tablets in an "update batch" (startUpdate -> applyUpdates ->
closeUpdate), we make a separate update call one at a time (see TabletServerLogger).

Yes yes, you're right. I think it would be better if we could share these across all updates
to tablets hosted by that tserver, but I would concur that it likely makes the problem a bit
harder. I also don't know if any other problems would arise by trying to implement this :)

Maybe it's not an issue across threads, but only the first time that the tablet writes the
new WAL record? If you have multiple updates coming in for the same tablet, only the first
one that gets applied would "scribble" that WAL use, right? Maybe my concern is completely
invalid then... More thought with a fresh mind needed by me.


- Josh


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


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


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