accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mike Drob" <md...@mdrob.com>
Subject Re: Review Request 22658: ACCUMULO-2889: Batch metadata updates for new WALs
Date Tue, 17 Jun 2014 16:46:15 GMT


> 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.
> 
> Jonathan Park wrote:
>     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.

Not sure I understand the alternative suggestion. Regardless of implementation, javadoc is
always welcome, though.


> 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.
> 
> Jonathan Park wrote:
>     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.

What are the exceptional conditions here? If the table does not exist, does it make sense
to just keep retrying? i.e. do we think some other process will create it eventually? Same
with security exception - do we believe that something else will alter our permisions/credentials?
At the very least, a 100ms sleep would be good after an error so that whatever condition caused
the error gets a chance to clean up.


> 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.
> 
> Jonathan Park wrote:
>     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?

I don't have strong opinions one way over the other, just thought that it would be nice to
consolidate code where it might make sense to do so. Adding javadoc to explain why a developer
should choose one method over the other would be good too.


> 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?
> 
> Jonathan Park wrote:
>     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).

Another thought I had was about hte interaction between batches and non-batched updates. Do
those still work correctly? Do they attempt to lock on the same objects?


- Mike


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