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