accumulo-dev mailing list archives

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


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


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